[{"id":11470,"web_url":"https://patchwork.libcamera.org/comment/11470/","msgid":"<4577acb0-3976-4721-6673-e456db4dc4b3@ideasonboard.com>","date":"2020-07-21T15:40:58","subject":"Re: [libcamera-devel] [PATCH v4 6/9] libcamera: pipeline:\n\traspberrypi: Rework stream buffer logic for zero-copy","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush,\n\nOn 20/07/2020 10:13, Naushir Patuck wrote:\n> Stop using v4l2_videodevice::allocateBuffer() for internal buffers and\n> instead export/import all buffers. This allows the pipeline to return\n> any stream buffer requested by the application as zero-copy.\n> \n> Advertise the Unicam Image stream as the RAW capture stream now.\n> \n> The RPiStream object now maintains a new list of buffers that are\n> available to queue into a device. This is needed to distinguish between\n> FrameBuffers allocated for internal use vs externally provided buffers.\n> When a Request comes in, if a buffer is not provided for an exported\n> stream, we re-use a buffer from this list.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: Niklas Söderlund \n\nA few small nit fixups below but they could possibly be fixed while\napplying anyway if you prefer.\n\nI've hit review fatigue though, so I'll look at this again tomorrow.\n\n\n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 228 ++++++++++--------\n> .../pipeline/raspberrypi/rpi_stream.cpp | 122 +++++++---\n> .../pipeline/raspberrypi/rpi_stream.h | 30 ++-\n> 3 files changed, 226 insertions(+), 154 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 8f6a999b..dbc22521 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -187,7 +187,8 @@ private:\n> \tvoid checkRequestCompleted();\n> \tvoid tryRunPipeline();\n> \tvoid tryFlushQueues();\n> -\tFrameBuffer *updateQueue(std::queue &q, uint64_t timestamp, V4L2VideoDevice *dev);\n> +\tFrameBuffer *updateQueue(std::queue &q, uint64_t timestamp,\n> +\t\t\t\t RPi::RPiStream *stream);\n> \n> \tunsigned int ispOutputCount_;\n> };\n> @@ -508,8 +509,15 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> \t\tStreamConfiguration &cfg = config->at(i);\n> \n> \t\tif (isRaw(cfg.pixelFormat)) {\n> -\t\t\tcfg.setStream(&data->isp_[Isp::Input]);\n> -\t\t\tdata->isp_[Isp::Input].setExternal(true);\n> +\t\t\tcfg.setStream(&data->unicam_[Unicam::Image]);\n> +\t\t\t/*\n> +\t\t\t * We must set both Unicam streams as external, even\n> +\t\t\t * though the application may only request RAW frames.\n> +\t\t\t * This is because we match timestamps on both streams\n> +\t\t\t * to synchronise buffers.\n> +\t\t\t */\n> +\t\t\tdata->unicam_[Unicam::Image].setExternal(true);\n> +\t\t\tdata->unicam_[Unicam::Embedded].setExternal(true);\n> \t\t\tcontinue;\n> \t\t}\n> \n> @@ -612,7 +620,7 @@ int PipelineHandlerRPi::exportFrameBuffers(Camera *camera, Stream *stream,\n> \tunsigned int count = stream->configuration().bufferCount;\n> \tint ret = s->dev()->exportBuffers(count, buffers);\n> \n> -\ts->setExternalBuffers(buffers);\n> +\ts->setExportedBuffers(buffers);\n> \n> \treturn ret;\n> }\n> @@ -712,14 +720,23 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> \tif (data->state_ == RPiCameraData::State::Stopped)\n> \t\treturn -EINVAL;\n> \n> -\t/* Ensure all external streams have associated buffers! */\n> -\tfor (auto &stream : data->isp_) {\n> -\t\tif (!stream.isExternal())\n> -\t\t\tcontinue;\n> +\tLOG(RPI, Debug) << \"queueRequestDevice: New request.\";\n> \n> -\t\tif (!request->findBuffer(&stream)) {\n> -\t\t\tLOG(RPI, Error) << \"Attempt to queue request with invalid stream.\";\n> -\t\t\treturn -ENOENT;\n> +\t/* Push all buffers supplied in the Request to the respective streams. */\n> +\tfor (auto stream : data->streams_) {\n> +\t\tif (stream->isExternal()) {\n\n\nAre all streams now 'external' ?\n\n\n> +\t\t\tFrameBuffer *buffer = request->findBuffer(stream);\n> +\t\t\t/*\n> +\t\t\t * A nullptr in buffer means that we should queue an internally\n> +\t\t\t * allocated buffer.\n> +\t\t\t *\n> +\t\t\t * The below queueBuffer() call will do nothing if there are not\n> +\t\t\t * enough internal buffers allocated, but this will be handled by\n> +\t\t\t * queuing the request for buffers in the RPiStream object.\n> +\t\t\t */\n> +\t\t\tint ret = stream->queueBuffer(buffer);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> \t\t}\n> \t}\n> \n> @@ -808,12 +825,10 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> \t/* Initialize the camera properties. */\n> \tdata->properties_ = data->sensor_->properties();\n> \n> -\t/*\n> -\t * List the available output streams.\n> -\t * Currently cannot do Unicam streams!\n> -\t */\n> +\t/*List the available streams an application may request. */\n\nMinor nit, there's a space missing between /* and List ...\n\n\n> \tstd::set streams;\n> -\tstreams.insert(&data->isp_[Isp::Input]);\n> +\tstreams.insert(&data->unicam_[Unicam::Image]);\n> +\tstreams.insert(&data->unicam_[Unicam::Embedded]);\n> \tstreams.insert(&data->isp_[Isp::Output0]);\n> \tstreams.insert(&data->isp_[Isp::Output1]);\n> \tstreams.insert(&data->isp_[Isp::Stats]);\n> @@ -831,9 +846,28 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> \tint ret;\n> \n> \tfor (auto const stream : data->streams_) {\n> -\t\tret = stream->queueBuffers();\n> -\t\tif (ret < 0)\n> -\t\t\treturn ret;\n> +\t\tif (!stream->isExternal()) {\n> +\t\t\tret = stream->queueAllBuffers();\n> +\t\t\tif (ret < 0)\n> +\t\t\t\treturn ret;\n> +\t\t} else {\n> +\t\t\t/*\n> +\t\t\t * For external streams, we must queue up a set of internal\n> +\t\t\t * buffers to handle the number of drop frames requested by\n> +\t\t\t * the IPA. This is done by passing nullptr in queueBuffer().\n> +\t\t\t *\n> +\t\t\t * The below queueBuffer() call will do nothing if there\n> +\t\t\t * are not enough internal buffers allocated, but this will\n> +\t\t\t * be handled by queuing the request for buffers in the\n> +\t\t\t * RPiStream object.\n> +\t\t\t */\n> +\t\t\tunsigned int i;\n> +\t\t\tfor (i = 0; i < data->dropFrameCount_; i++) {\n> +\t\t\t\tint ret = stream->queueBuffer(nullptr);\n> +\t\t\t\tif (ret)\n> +\t\t\t\t\treturn ret;\n> +\t\t\t}\n> +\t\t}\n> \t}\n> \n> \treturn 0;\n> @@ -847,7 +881,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> \t/*\n> \t * Decide how many internal buffers to allocate. For now, simply look\n> \t * at how many external buffers will be provided. Will need to improve\n> -\t * this logic.\n> +\t * this logic. However, we really must have all stream allocate the same\n\ns/stream/streams/\n\n> +\t * number of buffers to simplify error handling in queueRequestDevice().\n> \t */\n\nDoes this include Raw streams? I thought that allocates less buffers? Or\nperhaps it's the same number of internal buffers, and only 2 extra\nexternal buffers for that case...\n\n\n> \tunsigned int maxBuffers = 0;\n> \tfor (const Stream *s : camera->streams())\n> @@ -855,33 +890,9 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> \t\t\tmaxBuffers = std::max(maxBuffers, s->configuration().bufferCount);\n> \n> \tfor (auto const stream : data->streams_) {\n> -\t\tif (stream->isExternal() || stream->isImporter()) {\n> -\t\t\t/*\n> -\t\t\t * If a stream is marked as external reserve memory to\n> -\t\t\t * prepare to import as many buffers are requested in\n> -\t\t\t * the stream configuration.\n> -\t\t\t *\n> -\t\t\t * If a stream is an internal stream with importer\n> -\t\t\t * role, reserve as many buffers as possible.\n> -\t\t\t */\n> -\t\t\tunsigned int count = stream->isExternal()\n> -\t\t\t\t\t\t ? stream->configuration().bufferCount\n> -\t\t\t\t\t\t : maxBuffers;\n> -\t\t\tret = stream->importBuffers(count);\n> -\t\t\tif (ret < 0)\n> -\t\t\t\treturn ret;\n> -\t\t} else {\n> -\t\t\t/*\n> -\t\t\t * If the stream is an internal exporter allocate and\n> -\t\t\t * export as many buffers as possible to its internal\n> -\t\t\t * pool.\n> -\t\t\t */\n> -\t\t\tret = stream->allocateBuffers(maxBuffers);\n> -\t\t\tif (ret < 0) {\n> -\t\t\t\tfreeBuffers(camera);\n> -\t\t\t\treturn ret;\n> -\t\t\t}\n> -\t\t}\n> +\t\tret = stream->prepareBuffers(maxBuffers);\n> +\t\tif (ret < 0)\n> +\t\t\treturn ret;\n> \t}\n> \n> \t/*\n> @@ -889,7 +900,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> \t * the IPA and RPI_IPA_EVENT_SIGNAL_ISP_PREPARE event.\n> \t */\n> \tcount = 0;\n> -\tfor (auto const &b : *data->unicam_[Unicam::Image].getBuffers()) {\n> +\tfor (auto const &b : data->unicam_[Unicam::Image].getBuffers()) {\n> \t\tb->setCookie(count++);\n> \t}\n> \n> @@ -898,14 +909,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> \t * the IPA.\n> \t */\n> \tcount = 0;\n> -\tfor (auto const &b : *data->isp_[Isp::Stats].getBuffers()) {\n> +\tfor (auto const &b : data->isp_[Isp::Stats].getBuffers()) {\n> \t\tb->setCookie(count++);\n> \t\tdata->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b->cookie(),\n> \t\t\t\t\t .planes = b->planes() });\n> \t}\n> \n> \tcount = 0;\n> -\tfor (auto const &b : *data->unicam_[Unicam::Embedded].getBuffers()) {\n> +\tfor (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {\n> \t\tb->setCookie(count++);\n> \t\tdata->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b->cookie(),\n> \t\t\t\t\t .planes = b->planes() });\n> @@ -1066,7 +1077,7 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n> \tswitch (action.operation) {\n> \tcase RPI_IPA_ACTION_STATS_METADATA_COMPLETE: {\n> \t\tunsigned int bufferId = action.data[0];\n> -\t\tFrameBuffer *buffer = isp_[Isp::Stats].getBuffers()->at(bufferId).get();\n> +\t\tFrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> \n> \t\thandleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> \t\t/* Fill the Request metadata buffer with what the IPA has provided */\n> @@ -1077,19 +1088,19 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n> \n> \tcase RPI_IPA_ACTION_EMBEDDED_COMPLETE: {\n> \t\tunsigned int bufferId = action.data[0];\n> -\t\tFrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers()->at(bufferId).get();\n> +\t\tFrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);\n> \t\thandleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> \t\tbreak;\n> \t}\n> \n> \tcase RPI_IPA_ACTION_RUN_ISP: {\n> \t\tunsigned int bufferId = action.data[0];\n> -\t\tFrameBuffer *buffer = unicam_[Unicam::Image].getBuffers()->at(bufferId).get();\n> +\t\tFrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);\n> \n> \t\tLOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << buffer->cookie()\n> \t\t\t\t<< \", timestamp: \" << buffer->metadata().timestamp;\n> \n> -\t\tisp_[Isp::Input].dev()->queueBuffer(buffer);\n> +\t\tisp_[Isp::Input].queueBuffer(buffer);\n> \t\tispOutputCount_ = 0;\n> \t\tbreak;\n> \t}\n> @@ -1190,22 +1201,27 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n> \t\t\t<< \", buffer id \" << buffer->cookie()\n> \t\t\t<< \", timestamp: \" << buffer->metadata().timestamp;\n> \n> -\thandleStreamBuffer(buffer, stream);\n> -\n> \t/*\n> -\t * Increment the number of ISP outputs generated.\n> -\t * This is needed to track dropped frames.\n> +\t * ISP statistics buffer must not be re-queued or sent back to the\n> +\t * application until after the IPA signals so.\n> \t */\n> -\tispOutputCount_++;\n> -\n> -\t/* If this is a stats output, hand it to the IPA now. */\n> \tif (stream == &isp_[Isp::Stats]) {\n> \t\tIPAOperationData op;\n> \t\top.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;\n> \t\top.data = { RPiIpaMask::STATS | buffer->cookie() };\n> \t\tipa_->processEvent(op);\n> +\t} else {\n> +\t\t/* Any other ISP output can be handed back to the application now. */\n> +\t\thandleStreamBuffer(buffer, stream);\n> \t}\n> \n> +\t/*\n> +\t * Increment the number of ISP outputs generated.\n> +\t * This is needed to track dropped frames.\n> +\t */\n> +\tispOutputCount_++;\n> +\n> +\n> \thandleState();\n> }\n> \n> @@ -1218,8 +1234,12 @@ void RPiCameraData::clearIncompleteRequests()\n> \t */\n> \tfor (auto const request : requestQueue_) {\n> \t\tfor (auto const stream : streams_) {\n> -\t\t\tif (stream->isExternal())\n> -\t\t\t\tstream->dev()->queueBuffer(request->findBuffer(stream));\n> +\t\t\tif (!stream->isExternal())\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tFrameBuffer *buffer = request->findBuffer(stream);\n> +\t\t\tif (buffer)\n> +\t\t\t\tstream->queueBuffer(buffer);\n> \t\t}\n> \t}\n> \n> @@ -1248,7 +1268,7 @@ void RPiCameraData::clearIncompleteRequests()\n> \t\t\t * Has the buffer already been handed back to the\n> \t\t\t * request? If not, do so now.\n> \t\t\t */\n> -\t\t\tif (buffer->request())\n> +\t\t\tif (buffer && buffer->request())\n> \t\t\t\tpipe_->completeBuffer(camera_, request, buffer);\n> \t\t}\n> \n> @@ -1260,30 +1280,24 @@ void RPiCameraData::clearIncompleteRequests()\n> void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)\n> {\n> \tif (stream->isExternal()) {\n> -\t\tif (!dropFrameCount_) {\n> -\t\t\tRequest *request = buffer->request();\n> +\t\tRequest *request = requestQueue_.front();\n> +\n> +\t\tif (!dropFrameCount_ && request->findBuffer(stream) == buffer) {\n> +\t\t\t/*\n> +\t\t\t * Tag the buffer as completed, returning it to the\n> +\t\t\t * application.\n> +\t\t\t */\n> \t\t\tpipe_->completeBuffer(camera_, request, buffer);\n> +\t\t} else {\n> +\t\t\t/*\n> +\t\t\t * This buffer was not part of the Request, so we can\n> +\t\t\t * recycle it.\n> +\t\t\t */\n> +\t\t\tstream->returnBuffer(buffer);\n> \t\t}\n> \t} else {\n> -\t\t/* Special handling for RAW buffer Requests.\n> -\t\t *\n> -\t\t * The ISP input stream is alway an import stream, but if the\n> -\t\t * current Request has been made for a buffer on the stream,\n> -\t\t * simply memcpy to the Request buffer and requeue back to the\n> -\t\t * device.\n> -\t\t */\n> -\t\tif (stream == &unicam_[Unicam::Image] && !dropFrameCount_) {\n> -\t\t\tconst Stream *rawStream = static_cast(&isp_[Isp::Input]);\n> -\t\t\tRequest *request = requestQueue_.front();\n> -\t\t\tFrameBuffer *raw = request->findBuffer(const_cast(rawStream));\n> -\t\t\tif (raw) {\n> -\t\t\t\traw->copyFrom(buffer);\n> -\t\t\t\tpipe_->completeBuffer(camera_, request, raw);\n> -\t\t\t}\n> -\t\t}\n> -\n> -\t\t/* Simply requeue the buffer. */\n> -\t\tstream->dev()->queueBuffer(buffer);\n> +\t\t/* Simply re-queue the buffer to the requested stream. */\n> +\t\tstream->queueBuffer(buffer);\n> \t}\n> }\n> \n> @@ -1367,7 +1381,7 @@ void RPiCameraData::tryRunPipeline()\n> \t * current bayer buffer will be removed and re-queued to the driver.\n> \t */\n> \tembeddedBuffer = updateQueue(embeddedQueue_, bayerBuffer->metadata().timestamp,\n> -\t\t\t\t unicam_[Unicam::Embedded].dev());\n> +\t\t\t\t &unicam_[Unicam::Embedded]);\n> \n> \tif (!embeddedBuffer) {\n> \t\tLOG(RPI, Debug) << \"Could not find matching embedded buffer\";\n> @@ -1386,7 +1400,7 @@ void RPiCameraData::tryRunPipeline()\n> \n> \t\tembeddedBuffer = embeddedQueue_.front();\n> \t\tbayerBuffer = updateQueue(bayerQueue_, embeddedBuffer->metadata().timestamp,\n> -\t\t\t\t\t unicam_[Unicam::Image].dev());\n> +\t\t\t\t\t &unicam_[Unicam::Image]);\n> \n> \t\tif (!bayerBuffer) {\n> \t\t\tLOG(RPI, Debug) << \"Could not find matching bayer buffer - ending.\";\n> @@ -1394,11 +1408,7 @@ void RPiCameraData::tryRunPipeline()\n> \t\t}\n> \t}\n> \n> -\t/*\n> -\t * Take the first request from the queue and action the IPA.\n> -\t * Unicam buffers for the request have already been queued as they come\n> -\t * in.\n> -\t */\n> +\t/* Take the first request from the queue and action the IPA. */\n> \tRequest *request = requestQueue_.front();\n> \n> \t/*\n> @@ -1410,12 +1420,6 @@ void RPiCameraData::tryRunPipeline()\n> \top.controls = { request->controls() };\n> \tipa_->processEvent(op);\n> \n> -\t/* Queue up any ISP buffers passed into the request. */\n> -\tfor (auto &stream : isp_) {\n> -\t\tif (stream.isExternal())\n> -\t\t\tstream.dev()->queueBuffer(request->findBuffer(&stream));\n> -\t}\n> -\n> \t/* Ready to use the buffers, pop them off the queue. */\n> \tbayerQueue_.pop();\n> \tembeddedQueue_.pop();\n> @@ -1445,32 +1449,42 @@ void RPiCameraData::tryFlushQueues()\n> \t * and give a chance for the hardware to return to lock-step. We do have\n> \t * to drop all interim frames.\n> \t */\n> -\tif (unicam_[Unicam::Image].getBuffers()->size() == bayerQueue_.size() &&\n> -\t unicam_[Unicam::Embedded].getBuffers()->size() == embeddedQueue_.size()) {\n> +\tif (unicam_[Unicam::Image].getBuffers().size() == bayerQueue_.size() &&\n> +\t unicam_[Unicam::Embedded].getBuffers().size() == embeddedQueue_.size()) {\n> +\t\t/* This cannot happen when Unicam streams are external. */\n> +\t\tassert(!unicam_[Unicam::Image].isExternal());\n> +\n> \t\tLOG(RPI, Warning) << \"Flushing all buffer queues!\";\n> \n> \t\twhile (!bayerQueue_.empty()) {\n> -\t\t\tunicam_[Unicam::Image].dev()->queueBuffer(bayerQueue_.front());\n> +\t\t\tunicam_[Unicam::Image].queueBuffer(bayerQueue_.front());\n> \t\t\tbayerQueue_.pop();\n> \t\t}\n> \n> \t\twhile (!embeddedQueue_.empty()) {\n> -\t\t\tunicam_[Unicam::Embedded].dev()->queueBuffer(embeddedQueue_.front());\n> +\t\t\tunicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());\n> \t\t\tembeddedQueue_.pop();\n> \t\t}\n> \t}\n> }\n> \n> FrameBuffer *RPiCameraData::updateQueue(std::queue &q, uint64_t timestamp,\n> -\t\t\t\t\tV4L2VideoDevice *dev)\n> +\t\t\t\t\tRPi::RPiStream *stream)\n> {\n> +\t/*\n> +\t * If the unicam streams are external (both have to the same), then we\n\nboth have to 'be' the same?\n\n> +\t * can only return out the top buffer in the queue, and assume they have\n> +\t * been synced by queuing at the same time. We cannot drop these frames,\n> +\t * as they may have been provided externally.\n> +\t */\n> \twhile (!q.empty()) {\n> \t\tFrameBuffer *b = q.front();\n> -\t\tif (b->metadata().timestamp < timestamp) {\n> +\t\tif (!stream->isExternal() && b->metadata().timestamp < timestamp) {\n> \t\t\tq.pop();\n> -\t\t\tdev->queueBuffer(b);\n> -\t\t\tLOG(RPI, Warning) << \"Dropping input frame!\";\n> -\t\t} else if (b->metadata().timestamp == timestamp) {\n> +\t\t\tstream->queueBuffer(b);\n> +\t\t\tLOG(RPI, Warning) << \"Dropping unmatched input frame in stream \"\n> +\t\t\t\t\t << stream->name();\n> +\t\t} else if (stream->isExternal() || b->metadata().timestamp == timestamp) {\n> \t\t\t/* The calling function will pop the item from the queue. */\n> \t\t\treturn b;\n> \t\t} else {\n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> index 2edb8b59..02f8d3e0 100644\n> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> @@ -21,30 +21,20 @@ V4L2VideoDevice *RPiStream::dev() const\n> \n> void RPiStream::setExternal(bool external)\n> {\n> +\t/* Import streams cannot be external. */\n> +\tassert(!external || !importOnly_);\n> \texternal_ = external;\n> }\n> \n> bool RPiStream::isExternal() const\n> {\n> -\t/*\n> -\t * Import streams cannot be external.\n> -\t *\n> -\t * RAW capture is a special case where we simply copy the RAW\n> -\t * buffer out of the request. All other buffer handling happens\n> -\t * as if the stream is internal.\n> -\t */\n> -\treturn external_ && !importOnly_;\n> -}\n> -\n> -bool RPiStream::isImporter() const\n> -{\n> -\treturn importOnly_;\n> +\treturn external_;\n> }\n> \n> void RPiStream::reset()\n> {\n> \texternal_ = false;\n> -\tinternalBuffers_.clear();\n> +\tclearBuffers();\n> }\n> \n> std::string RPiStream::name() const\n> @@ -52,65 +42,123 @@ std::string RPiStream::name() const\n> \treturn name_;\n> }\n> \n> -void RPiStream::setExternalBuffers(std::vector> *buffers)\n> +void RPiStream::setExportedBuffers(std::vector> *buffers)\n> {\n> -\texternalBuffers_ = buffers;\n> +\tstd::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_),\n> +\t\t [](std::unique_ptr &b) { return b.get(); });\n> }\n> \n> -const std::vector> *RPiStream::getBuffers() const\n> +const std::vector &RPiStream::getBuffers() const\n> {\n> -\treturn external_ ? externalBuffers_ : &internalBuffers_;\n> +\treturn bufferList_;\n> }\n> \n> void RPiStream::releaseBuffers()\n> {\n> \tdev_->releaseBuffers();\n> -\tif (!external_ && !importOnly_)\n> -\t\tinternalBuffers_.clear();\n> +\tclearBuffers();\n> }\n> \n> -int RPiStream::importBuffers(unsigned int count)\n> +int RPiStream::prepareBuffers(unsigned int count)\n> {\n> +\tint ret;\n> +\n> +\tif (!importOnly_) {\n> +\t\tif (count) {\n> +\t\t\t/* Export some frame buffers for internal use. */\n> +\t\t\tret = dev_->exportBuffers(count, &internalBuffers_);\n> +\t\t\tif (ret < 0)\n> +\t\t\t\treturn ret;\n> +\n> +\t\t\t/* Add these exported buffers to the internal/external buffer list. */\n> +\t\t\tsetExportedBuffers(&internalBuffers_);\n> +\n> +\t\t\t/* Add these buffers to the queue of internal usable buffers. */\n> +\t\t\tfor (auto const &buffer : internalBuffers_)\n> +\t\t\t\tavailableBuffers_.push(buffer.get());\n> +\t\t}\n> +\n> +\t\t/* We must import all internal/external exported buffers. */\n> +\t\tcount = bufferList_.size();\n> +\t}\n> +\n> \treturn dev_->importBuffers(count);\n> }\n> \n> -int RPiStream::allocateBuffers(unsigned int count)\n> +int RPiStream::queueAllBuffers()\n> {\n> -\treturn dev_->allocateBuffers(count, &internalBuffers_);\n> -}\n> +\tint ret;\n> \n> -int RPiStream::queueBuffers()\n> -{\n> \tif (external_)\n> \t\treturn 0;\n> \n> -\tfor (auto &b : internalBuffers_) {\n> -\t\tint ret = dev_->queueBuffer(b.get());\n> -\t\tif (ret) {\n> -\t\t\tLOG(RPISTREAM, Error) << \"Failed to queue buffers for \"\n> -\t\t\t\t\t << name_;\n> +\twhile (!availableBuffers_.empty()) {\n> +\t\tret = queueBuffer(availableBuffers_.front());\n> +\t\tif (ret < 0)\n> \t\t\treturn ret;\n> -\t\t}\n> +\n> +\t\tavailableBuffers_.pop();\n> \t}\n> \n> \treturn 0;\n> }\n> \n> -bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const\n> +int RPiStream::queueBuffer(FrameBuffer *buffer)\n> +{\n> +\t/*\n> +\t * A nullptr buffer implies an external stream, but no external\n> +\t * buffer has been supplied. So, pick one from the availableBuffers_\n> +\t * queue.\n> +\t */\n> +\tif (!buffer) {\n> +\t\tif (availableBuffers_.empty()) {\n> +\t\t\tLOG(RPISTREAM, Warning) << \"No buffers available for \"\n> +\t\t\t\t\t\t<< name_;\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tbuffer = availableBuffers_.front();\n> +\t\tavailableBuffers_.pop();\n> +\t}\n> +\n> +\tLOG(RPISTREAM, Debug) << \"Queuing buffer \" << buffer->cookie()\n> +\t\t\t << \" for \" << name_;\n> +\n> +\tint ret = dev_->queueBuffer(buffer);\n> +\tif (ret) {\n> +\t\tLOG(RPISTREAM, Error) << \"Failed to queue buffer for \"\n> +\t\t\t\t << name_;\n> +\t}\n> +\n> +\treturn ret;\n> +}\n> +\n> +void RPiStream::returnBuffer(FrameBuffer *buffer)\n> {\n> -\tauto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();\n> -\tauto end = external_ ? externalBuffers_->end() : internalBuffers_.end();\n> +\t/* This can only be called for external streams. */\n> +\tassert(external_);\n> +\n> +\tavailableBuffers_.push(buffer);\n> +}\n> \n> +bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const\n> +{\n> \tif (importOnly_)\n> \t\treturn false;\n> \n> -\tif (std::find_if(start, end,\n> -\t\t\t [buffer](std::unique_ptr const &ref) { return ref.get() == buffer; }) != end)\n> +\tif (std::find(bufferList_.begin(), bufferList_.end(), buffer) != bufferList_.end())\n> \t\treturn true;\n> \n> \treturn false;\n> }\n> \n> +void RPiStream::clearBuffers()\n> +{\n> +\tavailableBuffers_ = std::queue{};\n> +\tinternalBuffers_.clear();\n> +\tbufferList_.clear();\n> +}\n> +\n> } /* namespace RPi */\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> index 3957e342..af9c2ad2 100644\n> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> @@ -39,21 +39,22 @@ public:\n> \tV4L2VideoDevice *dev() const;\n> \tvoid setExternal(bool external);\n> \tbool isExternal() const;\n> -\tbool isImporter() const;\n> \tvoid reset();\n> \tstd::string name() const;\n> -\tvoid setExternalBuffers(std::vector> *buffers);\n> -\tconst std::vector> *getBuffers() const;\n> +\tvoid setExportedBuffers(std::vector> *buffers);\n> +\tconst std::vector &getBuffers() const;\n> \tvoid releaseBuffers();\n> -\tint importBuffers(unsigned int count);\n> -\tint allocateBuffers(unsigned int count);\n> -\tint queueBuffers();\n> +\tint prepareBuffers(unsigned int count);\n> +\tint queueAllBuffers();\n> +\tint queueBuffer(FrameBuffer *buffer);\n> +\tvoid returnBuffer(FrameBuffer *buffer);\n> \tbool findFrameBuffer(FrameBuffer *buffer) const;\n\n\nPerhaps we should break those up into sections in patch 1/9. It's hard\nto see the wood-for-the-trees in the wall of function declarations.\n\n\n> \n> private:\n> +\tvoid clearBuffers();\n> \t/*\n> \t * Indicates that this stream is active externally, i.e. the buffers\n> -\t * are provided by the application.\n> +\t * might be provided by (and returned to) the application.\n> \t */\n> \tbool external_;\n> \t/* Indicates that this stream only imports buffers, e.g. ISP input. */\n> @@ -62,10 +63,19 @@ private:\n> \tstd::string name_;\n> \t/* The actual device stream. */\n> \tstd::unique_ptr dev_;\n> -\t/* Internally allocated framebuffers associated with this device stream. */\n> +\t/* All framebuffers associated with this device stream. */\n> +\tstd::vector bufferList_;\n> +\t/*\n> +\t * List of frame buffer that we can use if none have been provided by\n> +\t * the application for external streams. This is populated by the\n> +\t * buffers exported internally.\n> +\t */\n> +\tstd::queue availableBuffers_;\n> +\t/*\n> +\t * This is a list of buffers exported internally. Need to keep this around\n> +\t * as the stream needs to maintain ownership of these buffers.\n> +\t */\n> \tstd::vector> internalBuffers_;\n> -\t/* Externally allocated framebuffers associated with this device stream. */\n> -\tstd::vector> *externalBuffers_;\n\nLikewise here, some whitespace to break up these members would really\nhelp, as by the time I've got here I've got definite review fatigue and\nlots of lines close together just become a blur ... :S\n\n\n> };\n> \n> /*\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 84CFDC2E68\n\tfor ;\n\tTue, 21 Jul 2020 15:41:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E5F8F60930;\n\tTue, 21 Jul 2020 17:41:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DE3D860554\n\tfor ;\n\tTue, 21 Jul 2020 17:41:02 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B372F51A;\n\tTue, 21 Jul 2020 17:41:00 +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=\"g1RsZj0x\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1595346060;\n\tbh=FtVDZ6jgscAJ/G5FfUmoQOZKfwlb3jLyYfwxB4rcGyI=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=g1RsZj0xkZuEQIbml09rml5WeZhA6PB4ZMY/lOu6HBEXdEl/5ZQMgINaBL1g08El0\n\tRfFn6b6KumdhvOsIA91BBQQ5BzTuMmtmhPsarL2wSA1raslwzkjlpMzX2gACtTIIVe\n\tr+MmYc/7vssI9tkKtqkJrSPI7dUpX/YyTei+yp54=","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20200720091311.805092-1-naush@raspberrypi.com>\n\t<20200720091311.805092-7-naush@raspberrypi.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<4577acb0-3976-4721-6673-e456db4dc4b3@ideasonboard.com>","Date":"Tue, 21 Jul 2020 16:40:58 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20200720091311.805092-7-naush@raspberrypi.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v4 6/9] libcamera: pipeline:\n\traspberrypi: Rework stream buffer logic for zero-copy","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","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11474,"web_url":"https://patchwork.libcamera.org/comment/11474/","msgid":"","date":"2020-07-22T08:04:12","subject":"Re: [libcamera-devel] [PATCH v4 6/9] libcamera: pipeline:\n\traspberrypi: Rework stream buffer logic for zero-copy","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nThank you for the review.\n\nOn Tue, 21 Jul 2020 at 16:41, Kieran Bingham\n wrote:\n>\n> Hi Naush,\n>\n> On 20/07/2020 10:13, Naushir Patuck wrote:\n> > Stop using v4l2_videodevice::allocateBuffer() for internal buffers and\n> > instead export/import all buffers. This allows the pipeline to return\n> > any stream buffer requested by the application as zero-copy.\n> >\n> > Advertise the Unicam Image stream as the RAW capture stream now.\n> >\n> > The RPiStream object now maintains a new list of buffers that are\n> > available to queue into a device. This is needed to distinguish between\n> > FrameBuffers allocated for internal use vs externally provided buffers.\n> > When a Request comes in, if a buffer is not provided for an exported\n> > stream, we re-use a buffer from this list.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: Niklas Söderlund \n>\n> A few small nit fixups below but they could possibly be fixed while\n> applying anyway if you prefer.\n>\n> I've hit review fatigue though, so I'll look at this again tomorrow.\n\nI understand :)\nI will make the necessary fixups based on your review comments, and\nsubmit a new patch set. Will wait until you have finished reviewing\nthis one before I send the next revision.\n\n>\n>\n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 228 ++++++++++--------\n> > .../pipeline/raspberrypi/rpi_stream.cpp | 122 +++++++---\n> > .../pipeline/raspberrypi/rpi_stream.h | 30 ++-\n> > 3 files changed, 226 insertions(+), 154 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 8f6a999b..dbc22521 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -187,7 +187,8 @@ private:\n> > void checkRequestCompleted();\n> > void tryRunPipeline();\n> > void tryFlushQueues();\n> > - FrameBuffer *updateQueue(std::queue &q, uint64_t timestamp, V4L2VideoDevice *dev);\n> > + FrameBuffer *updateQueue(std::queue &q, uint64_t timestamp,\n> > + RPi::RPiStream *stream);\n> >\n> > unsigned int ispOutputCount_;\n> > };\n> > @@ -508,8 +509,15 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > StreamConfiguration &cfg = config->at(i);\n> >\n> > if (isRaw(cfg.pixelFormat)) {\n> > - cfg.setStream(&data->isp_[Isp::Input]);\n> > - data->isp_[Isp::Input].setExternal(true);\n> > + cfg.setStream(&data->unicam_[Unicam::Image]);\n> > + /*\n> > + * We must set both Unicam streams as external, even\n> > + * though the application may only request RAW frames.\n> > + * This is because we match timestamps on both streams\n> > + * to synchronise buffers.\n> > + */\n> > + data->unicam_[Unicam::Image].setExternal(true);\n> > + data->unicam_[Unicam::Embedded].setExternal(true);\n> > continue;\n> > }\n> >\n> > @@ -612,7 +620,7 @@ int PipelineHandlerRPi::exportFrameBuffers(Camera *camera, Stream *stream,\n> > unsigned int count = stream->configuration().bufferCount;\n> > int ret = s->dev()->exportBuffers(count, buffers);\n> >\n> > - s->setExternalBuffers(buffers);\n> > + s->setExportedBuffers(buffers);\n> >\n> > return ret;\n> > }\n> > @@ -712,14 +720,23 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> > if (data->state_ == RPiCameraData::State::Stopped)\n> > return -EINVAL;\n> >\n> > - /* Ensure all external streams have associated buffers! */\n> > - for (auto &stream : data->isp_) {\n> > - if (!stream.isExternal())\n> > - continue;\n> > + LOG(RPI, Debug) << \"queueRequestDevice: New request.\";\n> >\n> > - if (!request->findBuffer(&stream)) {\n> > - LOG(RPI, Error) << \"Attempt to queue request with invalid stream.\";\n> > - return -ENOENT;\n> > + /* Push all buffers supplied in the Request to the respective streams. */\n> > + for (auto stream : data->streams_) {\n> > + if (stream->isExternal()) {\n>\n>\n> Are all streams now 'external' ?\n\nAll streams but the ISP input (which only imports buffers) *can* be\nexternal. They are marked external only if the app configures the\npipeline handler by saying it may request a buffer from the stream.\nIn reality, all streams could be marked external all the time, but the\nbuffer handling will take a slightly less efficient path, so I kept\nthe distinction.\n\n>\n>\n> > + FrameBuffer *buffer = request->findBuffer(stream);\n> > + /*\n> > + * A nullptr in buffer means that we should queue an internally\n> > + * allocated buffer.\n> > + *\n> > + * The below queueBuffer() call will do nothing if there are not\n> > + * enough internal buffers allocated, but this will be handled by\n> > + * queuing the request for buffers in the RPiStream object.\n> > + */\n> > + int ret = stream->queueBuffer(buffer);\n> > + if (ret)\n> > + return ret;\n> > }\n> > }\n> >\n> > @@ -808,12 +825,10 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > /* Initialize the camera properties. */\n> > data->properties_ = data->sensor_->properties();\n> >\n> > - /*\n> > - * List the available output streams.\n> > - * Currently cannot do Unicam streams!\n> > - */\n> > + /*List the available streams an application may request. */\n>\n> Minor nit, there's a space missing between /* and List ...\n>\n>\n> > std::set streams;\n> > - streams.insert(&data->isp_[Isp::Input]);\n> > + streams.insert(&data->unicam_[Unicam::Image]);\n> > + streams.insert(&data->unicam_[Unicam::Embedded]);\n> > streams.insert(&data->isp_[Isp::Output0]);\n> > streams.insert(&data->isp_[Isp::Output1]);\n> > streams.insert(&data->isp_[Isp::Stats]);\n> > @@ -831,9 +846,28 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> > int ret;\n> >\n> > for (auto const stream : data->streams_) {\n> > - ret = stream->queueBuffers();\n> > - if (ret < 0)\n> > - return ret;\n> > + if (!stream->isExternal()) {\n> > + ret = stream->queueAllBuffers();\n> > + if (ret < 0)\n> > + return ret;\n> > + } else {\n> > + /*\n> > + * For external streams, we must queue up a set of internal\n> > + * buffers to handle the number of drop frames requested by\n> > + * the IPA. This is done by passing nullptr in queueBuffer().\n> > + *\n> > + * The below queueBuffer() call will do nothing if there\n> > + * are not enough internal buffers allocated, but this will\n> > + * be handled by queuing the request for buffers in the\n> > + * RPiStream object.\n> > + */\n> > + unsigned int i;\n> > + for (i = 0; i < data->dropFrameCount_; i++) {\n> > + int ret = stream->queueBuffer(nullptr);\n> > + if (ret)\n> > + return ret;\n> > + }\n> > + }\n> > }\n> >\n> > return 0;\n> > @@ -847,7 +881,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > /*\n> > * Decide how many internal buffers to allocate. For now, simply look\n> > * at how many external buffers will be provided. Will need to improve\n> > - * this logic.\n> > + * this logic. However, we really must have all stream allocate the same\n>\n> s/stream/streams/\n>\n> > + * number of buffers to simplify error handling in queueRequestDevice().\n> > */\n>\n> Does this include Raw streams? I thought that allocates less buffers? Or\n> perhaps it's the same number of internal buffers, and only 2 extra\n> external buffers for that case...\n\nThat's correct. For now all (including Raw) streams allocate the same\nnumber of internal buffers. Hence the comment that we could improve\nthis logic, it is a bit wasteful on memory. This bit does need\nrefinement at some point.\n\n>\n>\n> > unsigned int maxBuffers = 0;\n> > for (const Stream *s : camera->streams())\n> > @@ -855,33 +890,9 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);\n> >\n> > for (auto const stream : data->streams_) {\n> > - if (stream->isExternal() || stream->isImporter()) {\n> > - /*\n> > - * If a stream is marked as external reserve memory to\n> > - * prepare to import as many buffers are requested in\n> > - * the stream configuration.\n> > - *\n> > - * If a stream is an internal stream with importer\n> > - * role, reserve as many buffers as possible.\n> > - */\n> > - unsigned int count = stream->isExternal()\n> > - ? stream->configuration().bufferCount\n> > - : maxBuffers;\n> > - ret = stream->importBuffers(count);\n> > - if (ret < 0)\n> > - return ret;\n> > - } else {\n> > - /*\n> > - * If the stream is an internal exporter allocate and\n> > - * export as many buffers as possible to its internal\n> > - * pool.\n> > - */\n> > - ret = stream->allocateBuffers(maxBuffers);\n> > - if (ret < 0) {\n> > - freeBuffers(camera);\n> > - return ret;\n> > - }\n> > - }\n> > + ret = stream->prepareBuffers(maxBuffers);\n> > + if (ret < 0)\n> > + return ret;\n> > }\n> >\n> > /*\n> > @@ -889,7 +900,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > * the IPA and RPI_IPA_EVENT_SIGNAL_ISP_PREPARE event.\n> > */\n> > count = 0;\n> > - for (auto const &b : *data->unicam_[Unicam::Image].getBuffers()) {\n> > + for (auto const &b : data->unicam_[Unicam::Image].getBuffers()) {\n> > b->setCookie(count++);\n> > }\n> >\n> > @@ -898,14 +909,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > * the IPA.\n> > */\n> > count = 0;\n> > - for (auto const &b : *data->isp_[Isp::Stats].getBuffers()) {\n> > + for (auto const &b : data->isp_[Isp::Stats].getBuffers()) {\n> > b->setCookie(count++);\n> > data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b->cookie(),\n> > .planes = b->planes() });\n> > }\n> >\n> > count = 0;\n> > - for (auto const &b : *data->unicam_[Unicam::Embedded].getBuffers()) {\n> > + for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {\n> > b->setCookie(count++);\n> > data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b->cookie(),\n> > .planes = b->planes() });\n> > @@ -1066,7 +1077,7 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n> > switch (action.operation) {\n> > case RPI_IPA_ACTION_STATS_METADATA_COMPLETE: {\n> > unsigned int bufferId = action.data[0];\n> > - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers()->at(bufferId).get();\n> > + FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> >\n> > handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> > /* Fill the Request metadata buffer with what the IPA has provided */\n> > @@ -1077,19 +1088,19 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n> >\n> > case RPI_IPA_ACTION_EMBEDDED_COMPLETE: {\n> > unsigned int bufferId = action.data[0];\n> > - FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers()->at(bufferId).get();\n> > + FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);\n> > handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> > break;\n> > }\n> >\n> > case RPI_IPA_ACTION_RUN_ISP: {\n> > unsigned int bufferId = action.data[0];\n> > - FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers()->at(bufferId).get();\n> > + FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);\n> >\n> > LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << buffer->cookie()\n> > << \", timestamp: \" << buffer->metadata().timestamp;\n> >\n> > - isp_[Isp::Input].dev()->queueBuffer(buffer);\n> > + isp_[Isp::Input].queueBuffer(buffer);\n> > ispOutputCount_ = 0;\n> > break;\n> > }\n> > @@ -1190,22 +1201,27 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n> > << \", buffer id \" << buffer->cookie()\n> > << \", timestamp: \" << buffer->metadata().timestamp;\n> >\n> > - handleStreamBuffer(buffer, stream);\n> > -\n> > /*\n> > - * Increment the number of ISP outputs generated.\n> > - * This is needed to track dropped frames.\n> > + * ISP statistics buffer must not be re-queued or sent back to the\n> > + * application until after the IPA signals so.\n> > */\n> > - ispOutputCount_++;\n> > -\n> > - /* If this is a stats output, hand it to the IPA now. */\n> > if (stream == &isp_[Isp::Stats]) {\n> > IPAOperationData op;\n> > op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;\n> > op.data = { RPiIpaMask::STATS | buffer->cookie() };\n> > ipa_->processEvent(op);\n> > + } else {\n> > + /* Any other ISP output can be handed back to the application now. */\n> > + handleStreamBuffer(buffer, stream);\n> > }\n> >\n> > + /*\n> > + * Increment the number of ISP outputs generated.\n> > + * This is needed to track dropped frames.\n> > + */\n> > + ispOutputCount_++;\n> > +\n> > +\n> > handleState();\n> > }\n> >\n> > @@ -1218,8 +1234,12 @@ void RPiCameraData::clearIncompleteRequests()\n> > */\n> > for (auto const request : requestQueue_) {\n> > for (auto const stream : streams_) {\n> > - if (stream->isExternal())\n> > - stream->dev()->queueBuffer(request->findBuffer(stream));\n> > + if (!stream->isExternal())\n> > + continue;\n> > +\n> > + FrameBuffer *buffer = request->findBuffer(stream);\n> > + if (buffer)\n> > + stream->queueBuffer(buffer);\n> > }\n> > }\n> >\n> > @@ -1248,7 +1268,7 @@ void RPiCameraData::clearIncompleteRequests()\n> > * Has the buffer already been handed back to the\n> > * request? If not, do so now.\n> > */\n> > - if (buffer->request())\n> > + if (buffer && buffer->request())\n> > pipe_->completeBuffer(camera_, request, buffer);\n> > }\n> >\n> > @@ -1260,30 +1280,24 @@ void RPiCameraData::clearIncompleteRequests()\n> > void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)\n> > {\n> > if (stream->isExternal()) {\n> > - if (!dropFrameCount_) {\n> > - Request *request = buffer->request();\n> > + Request *request = requestQueue_.front();\n> > +\n> > + if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {\n> > + /*\n> > + * Tag the buffer as completed, returning it to the\n> > + * application.\n> > + */\n> > pipe_->completeBuffer(camera_, request, buffer);\n> > + } else {\n> > + /*\n> > + * This buffer was not part of the Request, so we can\n> > + * recycle it.\n> > + */\n> > + stream->returnBuffer(buffer);\n> > }\n> > } else {\n> > - /* Special handling for RAW buffer Requests.\n> > - *\n> > - * The ISP input stream is alway an import stream, but if the\n> > - * current Request has been made for a buffer on the stream,\n> > - * simply memcpy to the Request buffer and requeue back to the\n> > - * device.\n> > - */\n> > - if (stream == &unicam_[Unicam::Image] && !dropFrameCount_) {\n> > - const Stream *rawStream = static_cast(&isp_[Isp::Input]);\n> > - Request *request = requestQueue_.front();\n> > - FrameBuffer *raw = request->findBuffer(const_cast(rawStream));\n> > - if (raw) {\n> > - raw->copyFrom(buffer);\n> > - pipe_->completeBuffer(camera_, request, raw);\n> > - }\n> > - }\n> > -\n> > - /* Simply requeue the buffer. */\n> > - stream->dev()->queueBuffer(buffer);\n> > + /* Simply re-queue the buffer to the requested stream. */\n> > + stream->queueBuffer(buffer);\n> > }\n> > }\n> >\n> > @@ -1367,7 +1381,7 @@ void RPiCameraData::tryRunPipeline()\n> > * current bayer buffer will be removed and re-queued to the driver.\n> > */\n> > embeddedBuffer = updateQueue(embeddedQueue_, bayerBuffer->metadata().timestamp,\n> > - unicam_[Unicam::Embedded].dev());\n> > + &unicam_[Unicam::Embedded]);\n> >\n> > if (!embeddedBuffer) {\n> > LOG(RPI, Debug) << \"Could not find matching embedded buffer\";\n> > @@ -1386,7 +1400,7 @@ void RPiCameraData::tryRunPipeline()\n> >\n> > embeddedBuffer = embeddedQueue_.front();\n> > bayerBuffer = updateQueue(bayerQueue_, embeddedBuffer->metadata().timestamp,\n> > - unicam_[Unicam::Image].dev());\n> > + &unicam_[Unicam::Image]);\n> >\n> > if (!bayerBuffer) {\n> > LOG(RPI, Debug) << \"Could not find matching bayer buffer - ending.\";\n> > @@ -1394,11 +1408,7 @@ void RPiCameraData::tryRunPipeline()\n> > }\n> > }\n> >\n> > - /*\n> > - * Take the first request from the queue and action the IPA.\n> > - * Unicam buffers for the request have already been queued as they come\n> > - * in.\n> > - */\n> > + /* Take the first request from the queue and action the IPA. */\n> > Request *request = requestQueue_.front();\n> >\n> > /*\n> > @@ -1410,12 +1420,6 @@ void RPiCameraData::tryRunPipeline()\n> > op.controls = { request->controls() };\n> > ipa_->processEvent(op);\n> >\n> > - /* Queue up any ISP buffers passed into the request. */\n> > - for (auto &stream : isp_) {\n> > - if (stream.isExternal())\n> > - stream.dev()->queueBuffer(request->findBuffer(&stream));\n> > - }\n> > -\n> > /* Ready to use the buffers, pop them off the queue. */\n> > bayerQueue_.pop();\n> > embeddedQueue_.pop();\n> > @@ -1445,32 +1449,42 @@ void RPiCameraData::tryFlushQueues()\n> > * and give a chance for the hardware to return to lock-step. We do have\n> > * to drop all interim frames.\n> > */\n> > - if (unicam_[Unicam::Image].getBuffers()->size() == bayerQueue_.size() &&\n> > - unicam_[Unicam::Embedded].getBuffers()->size() == embeddedQueue_.size()) {\n> > + if (unicam_[Unicam::Image].getBuffers().size() == bayerQueue_.size() &&\n> > + unicam_[Unicam::Embedded].getBuffers().size() == embeddedQueue_.size()) {\n> > + /* This cannot happen when Unicam streams are external. */\n> > + assert(!unicam_[Unicam::Image].isExternal());\n> > +\n> > LOG(RPI, Warning) << \"Flushing all buffer queues!\";\n> >\n> > while (!bayerQueue_.empty()) {\n> > - unicam_[Unicam::Image].dev()->queueBuffer(bayerQueue_.front());\n> > + unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());\n> > bayerQueue_.pop();\n> > }\n> >\n> > while (!embeddedQueue_.empty()) {\n> > - unicam_[Unicam::Embedded].dev()->queueBuffer(embeddedQueue_.front());\n> > + unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());\n> > embeddedQueue_.pop();\n> > }\n> > }\n> > }\n> >\n> > FrameBuffer *RPiCameraData::updateQueue(std::queue &q, uint64_t timestamp,\n> > - V4L2VideoDevice *dev)\n> > + RPi::RPiStream *stream)\n> > {\n> > + /*\n> > + * If the unicam streams are external (both have to the same), then we\n>\n> both have to 'be' the same?\n>\n> > + * can only return out the top buffer in the queue, and assume they have\n> > + * been synced by queuing at the same time. We cannot drop these frames,\n> > + * as they may have been provided externally.\n> > + */\n> > while (!q.empty()) {\n> > FrameBuffer *b = q.front();\n> > - if (b->metadata().timestamp < timestamp) {\n> > + if (!stream->isExternal() && b->metadata().timestamp < timestamp) {\n> > q.pop();\n> > - dev->queueBuffer(b);\n> > - LOG(RPI, Warning) << \"Dropping input frame!\";\n> > - } else if (b->metadata().timestamp == timestamp) {\n> > + stream->queueBuffer(b);\n> > + LOG(RPI, Warning) << \"Dropping unmatched input frame in stream \"\n> > + << stream->name();\n> > + } else if (stream->isExternal() || b->metadata().timestamp == timestamp) {\n> > /* The calling function will pop the item from the queue. */\n> > return b;\n> > } else {\n> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > index 2edb8b59..02f8d3e0 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > @@ -21,30 +21,20 @@ V4L2VideoDevice *RPiStream::dev() const\n> >\n> > void RPiStream::setExternal(bool external)\n> > {\n> > + /* Import streams cannot be external. */\n> > + assert(!external || !importOnly_);\n> > external_ = external;\n> > }\n> >\n> > bool RPiStream::isExternal() const\n> > {\n> > - /*\n> > - * Import streams cannot be external.\n> > - *\n> > - * RAW capture is a special case where we simply copy the RAW\n> > - * buffer out of the request. All other buffer handling happens\n> > - * as if the stream is internal.\n> > - */\n> > - return external_ && !importOnly_;\n> > -}\n> > -\n> > -bool RPiStream::isImporter() const\n> > -{\n> > - return importOnly_;\n> > + return external_;\n> > }\n> >\n> > void RPiStream::reset()\n> > {\n> > external_ = false;\n> > - internalBuffers_.clear();\n> > + clearBuffers();\n> > }\n> >\n> > std::string RPiStream::name() const\n> > @@ -52,65 +42,123 @@ std::string RPiStream::name() const\n> > return name_;\n> > }\n> >\n> > -void RPiStream::setExternalBuffers(std::vector> *buffers)\n> > +void RPiStream::setExportedBuffers(std::vector> *buffers)\n> > {\n> > - externalBuffers_ = buffers;\n> > + std::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_),\n> > + [](std::unique_ptr &b) { return b.get(); });\n> > }\n> >\n> > -const std::vector> *RPiStream::getBuffers() const\n> > +const std::vector &RPiStream::getBuffers() const\n> > {\n> > - return external_ ? externalBuffers_ : &internalBuffers_;\n> > + return bufferList_;\n> > }\n> >\n> > void RPiStream::releaseBuffers()\n> > {\n> > dev_->releaseBuffers();\n> > - if (!external_ && !importOnly_)\n> > - internalBuffers_.clear();\n> > + clearBuffers();\n> > }\n> >\n> > -int RPiStream::importBuffers(unsigned int count)\n> > +int RPiStream::prepareBuffers(unsigned int count)\n> > {\n> > + int ret;\n> > +\n> > + if (!importOnly_) {\n> > + if (count) {\n> > + /* Export some frame buffers for internal use. */\n> > + ret = dev_->exportBuffers(count, &internalBuffers_);\n> > + if (ret < 0)\n> > + return ret;\n> > +\n> > + /* Add these exported buffers to the internal/external buffer list. */\n> > + setExportedBuffers(&internalBuffers_);\n> > +\n> > + /* Add these buffers to the queue of internal usable buffers. */\n> > + for (auto const &buffer : internalBuffers_)\n> > + availableBuffers_.push(buffer.get());\n> > + }\n> > +\n> > + /* We must import all internal/external exported buffers. */\n> > + count = bufferList_.size();\n> > + }\n> > +\n> > return dev_->importBuffers(count);\n> > }\n> >\n> > -int RPiStream::allocateBuffers(unsigned int count)\n> > +int RPiStream::queueAllBuffers()\n> > {\n> > - return dev_->allocateBuffers(count, &internalBuffers_);\n> > -}\n> > + int ret;\n> >\n> > -int RPiStream::queueBuffers()\n> > -{\n> > if (external_)\n> > return 0;\n> >\n> > - for (auto &b : internalBuffers_) {\n> > - int ret = dev_->queueBuffer(b.get());\n> > - if (ret) {\n> > - LOG(RPISTREAM, Error) << \"Failed to queue buffers for \"\n> > - << name_;\n> > + while (!availableBuffers_.empty()) {\n> > + ret = queueBuffer(availableBuffers_.front());\n> > + if (ret < 0)\n> > return ret;\n> > - }\n> > +\n> > + availableBuffers_.pop();\n> > }\n> >\n> > return 0;\n> > }\n> >\n> > -bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const\n> > +int RPiStream::queueBuffer(FrameBuffer *buffer)\n> > +{\n> > + /*\n> > + * A nullptr buffer implies an external stream, but no external\n> > + * buffer has been supplied. So, pick one from the availableBuffers_\n> > + * queue.\n> > + */\n> > + if (!buffer) {\n> > + if (availableBuffers_.empty()) {\n> > + LOG(RPISTREAM, Warning) << \"No buffers available for \"\n> > + << name_;\n> > + return -EINVAL;\n> > + }\n> > +\n> > + buffer = availableBuffers_.front();\n> > + availableBuffers_.pop();\n> > + }\n> > +\n> > + LOG(RPISTREAM, Debug) << \"Queuing buffer \" << buffer->cookie()\n> > + << \" for \" << name_;\n> > +\n> > + int ret = dev_->queueBuffer(buffer);\n> > + if (ret) {\n> > + LOG(RPISTREAM, Error) << \"Failed to queue buffer for \"\n> > + << name_;\n> > + }\n> > +\n> > + return ret;\n> > +}\n> > +\n> > +void RPiStream::returnBuffer(FrameBuffer *buffer)\n> > {\n> > - auto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();\n> > - auto end = external_ ? externalBuffers_->end() : internalBuffers_.end();\n> > + /* This can only be called for external streams. */\n> > + assert(external_);\n> > +\n> > + availableBuffers_.push(buffer);\n> > +}\n> >\n> > +bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const\n> > +{\n> > if (importOnly_)\n> > return false;\n> >\n> > - if (std::find_if(start, end,\n> > - [buffer](std::unique_ptr const &ref) { return ref.get() == buffer; }) != end)\n> > + if (std::find(bufferList_.begin(), bufferList_.end(), buffer) != bufferList_.end())\n> > return true;\n> >\n> > return false;\n> > }\n> >\n> > +void RPiStream::clearBuffers()\n> > +{\n> > + availableBuffers_ = std::queue{};\n> > + internalBuffers_.clear();\n> > + bufferList_.clear();\n> > +}\n> > +\n> > } /* namespace RPi */\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > index 3957e342..af9c2ad2 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > @@ -39,21 +39,22 @@ public:\n> > V4L2VideoDevice *dev() const;\n> > void setExternal(bool external);\n> > bool isExternal() const;\n> > - bool isImporter() const;\n> > void reset();\n> > std::string name() const;\n> > - void setExternalBuffers(std::vector> *buffers);\n> > - const std::vector> *getBuffers() const;\n> > + void setExportedBuffers(std::vector> *buffers);\n> > + const std::vector &getBuffers() const;\n> > void releaseBuffers();\n> > - int importBuffers(unsigned int count);\n> > - int allocateBuffers(unsigned int count);\n> > - int queueBuffers();\n> > + int prepareBuffers(unsigned int count);\n> > + int queueAllBuffers();\n> > + int queueBuffer(FrameBuffer *buffer);\n> > + void returnBuffer(FrameBuffer *buffer);\n> > bool findFrameBuffer(FrameBuffer *buffer) const;\n>\n>\n> Perhaps we should break those up into sections in patch 1/9. It's hard\n> to see the wood-for-the-trees in the wall of function declarations.\n\nNot sure I follow what you mean?\n\n>\n>\n> >\n> > private:\n> > + void clearBuffers();\n> > /*\n> > * Indicates that this stream is active externally, i.e. the buffers\n> > - * are provided by the application.\n> > + * might be provided by (and returned to) the application.\n> > */\n> > bool external_;\n> > /* Indicates that this stream only imports buffers, e.g. ISP input. */\n> > @@ -62,10 +63,19 @@ private:\n> > std::string name_;\n> > /* The actual device stream. */\n> > std::unique_ptr dev_;\n> > - /* Internally allocated framebuffers associated with this device stream. */\n> > + /* All framebuffers associated with this device stream. */\n> > + std::vector bufferList_;\n> > + /*\n> > + * List of frame buffer that we can use if none have been provided by\n> > + * the application for external streams. This is populated by the\n> > + * buffers exported internally.\n> > + */\n> > + std::queue availableBuffers_;\n> > + /*\n> > + * This is a list of buffers exported internally. Need to keep this around\n> > + * as the stream needs to maintain ownership of these buffers.\n> > + */\n> > std::vector> internalBuffers_;\n> > - /* Externally allocated framebuffers associated with this device stream. */\n> > - std::vector> *externalBuffers_;\n>\n> Likewise here, some whitespace to break up these members would really\n> help, as by the time I've got here I've got definite review fatigue and\n> lots of lines close together just become a blur ... :S\n\nThat's not a problem.\n\n>\n>\n> > };\n> >\n> > /*\n> >\n>\n> --\n> Regards\n> --\n> Kieran\n\nRegards,\nNaush","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 A22E5C2E68\n\tfor ;\n\tWed, 22 Jul 2020 08:04:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1C72F6098D;\n\tWed, 22 Jul 2020 10:04:31 +0200 (CEST)","from mail-lj1-x22c.google.com (mail-lj1-x22c.google.com\n\t[IPv6:2a00:1450:4864:20::22c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DA95660540\n\tfor ;\n\tWed, 22 Jul 2020 10:04:29 +0200 (CEST)","by mail-lj1-x22c.google.com with SMTP id q6so1507454ljp.4\n\tfor ;\n\tWed, 22 Jul 2020 01:04:29 -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=\"OerkCPAm\"; 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=LHsmOtijCgGew6aSI3tgsxEkWjGT58Sn3XkZk4C7rH0=;\n\tb=OerkCPAm3ZGEsHDZ2No8KhvteWi6zVWl/8/yynzZ7AMnHY8jmEZQUhWogsMKmSvdiF\n\tji3W9nH5bBHFLhClxmzGFrTM+InNLJ5Ukx6QEis6BxJg+x8NHsnQw4dNIgLqVu6ITmfM\n\tmTq848rJ7yDDhLfOI+ZBAhbc8yQ0J5vqrey+eQa+mWPirKLvc/6Zmkbz4VzhfWMo6GSX\n\tAkCvHApYKL1K5SNkgofvtbMvNOWrGgec13sSUdK9n+qbL0PFGvBd6JToasPv0XYPDnXh\n\tk9znVE4ZQyz0cX78qEpaoSnE+lLhOvJzsVw5MBWdeMKVCmpk9P/X1vX+9r/mAWhOPrdj\n\tnVmA==","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=LHsmOtijCgGew6aSI3tgsxEkWjGT58Sn3XkZk4C7rH0=;\n\tb=h1Srt2a5o2D2/l1NEZnjKokjJFvBItzFG111BAx+9mF6/ArBB3XrUpdHEysMK2WmOE\n\tYO1TsFQw313c64ONCPFOZ+wX3Q8Ucn8wl/6PnXkLuaq/VsqoZOEBygfGKGZx4ZAHxlRJ\n\tkPmsscwgE6h/djPw7m1ADLE0b8avOWmk18ddT8Kk7wYVnhowVv/J3f6EK1g4qFoNITl4\n\tcKWjRq77D6xRDcQDwQxys0gNaCBwuawRvhAjJGFHGFQY6tUxl2SQZvXV63mCvk0qMLkd\n\tGATvz/FwX7RPLC1Q9oA6CPE2eEmn18thgjS8kIIK2Vl9+CwnPQcuXN97TgCdY4VF46oR\n\tvuvw==","X-Gm-Message-State":"AOAM533CUcg/ciRv8D4dj0SY2FYO7jcxIKTfKTJSNf+JTjrIX/B/RnNF\n\t6aaeUs4sv5iLUrb9K+R+Bx749x/iK6tOwwKCbLlEM85h1J4=","X-Google-Smtp-Source":"ABdhPJw0nxdfUfifkriK/MBJ8az2hu+kJPqkfZSYwT0TFrb0fNOz5mTNhncVCOz35ZnlqSW51+uS21uC1L1hFO/qZok=","X-Received":"by 2002:a2e:b042:: with SMTP id\n\td2mr15082004ljl.252.1595405068790; \n\tWed, 22 Jul 2020 01:04:28 -0700 (PDT)","MIME-Version":"1.0","References":"<20200720091311.805092-1-naush@raspberrypi.com>\n\t<20200720091311.805092-7-naush@raspberrypi.com>\n\t<4577acb0-3976-4721-6673-e456db4dc4b3@ideasonboard.com>","In-Reply-To":"<4577acb0-3976-4721-6673-e456db4dc4b3@ideasonboard.com>","From":"Naushir Patuck ","Date":"Wed, 22 Jul 2020 09:04:12 +0100","Message-ID":"","To":"Kieran Bingham ","Subject":"Re: [libcamera-devel] [PATCH v4 6/9] libcamera: pipeline:\n\traspberrypi: Rework stream buffer logic for zero-copy","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11486,"web_url":"https://patchwork.libcamera.org/comment/11486/","msgid":"<2a259e2a-9cb6-73c3-26f6-328a78077b48@ideasonboard.com>","date":"2020-07-22T12:21:33","subject":"Re: [libcamera-devel] [PATCH v4 6/9] libcamera: pipeline:\n\traspberrypi: Rework stream buffer logic for zero-copy","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush,\n\nOn 22/07/2020 09:04, Naushir Patuck wrote:\n> Hi Kieran,\n> \n> Thank you for the review.\n> \n> On Tue, 21 Jul 2020 at 16:41, Kieran Bingham\n> wrote:\n>>\n>> Hi Naush,\n>>\n>> On 20/07/2020 10:13, Naushir Patuck wrote:\n>>> Stop using v4l2_videodevice::allocateBuffer() for internal buffers and\n>>> instead export/import all buffers. This allows the pipeline to return\n>>> any stream buffer requested by the application as zero-copy.\n>>>\n>>> Advertise the Unicam Image stream as the RAW capture stream now.\n>>>\n>>> The RPiStream object now maintains a new list of buffers that are\n>>> available to queue into a device. This is needed to distinguish between\n>>> FrameBuffers allocated for internal use vs externally provided buffers.\n>>> When a Request comes in, if a buffer is not provided for an exported\n>>> stream, we re-use a buffer from this list.\n>>>\n>>> Signed-off-by: Naushir Patuck \n>>> Reviewed-by: Niklas Söderlund \n>>\n>> A few small nit fixups below but they could possibly be fixed while\n>> applying anyway if you prefer.\n>>\n>> I've hit review fatigue though, so I'll look at this again tomorrow.\n> \n> I understand :)\n> I will make the necessary fixups based on your review comments, and\n> submit a new patch set. Will wait until you have finished reviewing\n> this one before I send the next revision.\n\nLooking good, I think the only actionable thing remaining here is\npotentially to use ASSERT() over assert() :-)\n\n\n> \n>>\n>>\n>>> ---\n>>> .../pipeline/raspberrypi/raspberrypi.cpp | 228 ++++++++++--------\n>>> .../pipeline/raspberrypi/rpi_stream.cpp | 122 +++++++---\n>>> .../pipeline/raspberrypi/rpi_stream.h | 30 ++-\n>>> 3 files changed, 226 insertions(+), 154 deletions(-)\n>>>\n>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> index 8f6a999b..dbc22521 100644\n>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> @@ -187,7 +187,8 @@ private:\n>>> void checkRequestCompleted();\n>>> void tryRunPipeline();\n>>> void tryFlushQueues();\n>>> - FrameBuffer *updateQueue(std::queue &q, uint64_t timestamp, V4L2VideoDevice *dev);\n>>> + FrameBuffer *updateQueue(std::queue &q, uint64_t timestamp,\n>>> + RPi::RPiStream *stream);\n>>>\n>>> unsigned int ispOutputCount_;\n>>> };\n>>> @@ -508,8 +509,15 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n>>> StreamConfiguration &cfg = config->at(i);\n>>>\n>>> if (isRaw(cfg.pixelFormat)) {\n>>> - cfg.setStream(&data->isp_[Isp::Input]);\n>>> - data->isp_[Isp::Input].setExternal(true);\n>>> + cfg.setStream(&data->unicam_[Unicam::Image]);\n>>> + /*\n>>> + * We must set both Unicam streams as external, even\n>>> + * though the application may only request RAW frames.\n>>> + * This is because we match timestamps on both streams\n>>> + * to synchronise buffers.\n>>> + */\n>>> + data->unicam_[Unicam::Image].setExternal(true);\n>>> + data->unicam_[Unicam::Embedded].setExternal(true);\n>>> continue;\n>>> }\n>>>\n>>> @@ -612,7 +620,7 @@ int PipelineHandlerRPi::exportFrameBuffers(Camera *camera, Stream *stream,\n>>> unsigned int count = stream->configuration().bufferCount;\n>>> int ret = s->dev()->exportBuffers(count, buffers);\n>>>\n>>> - s->setExternalBuffers(buffers);\n>>> + s->setExportedBuffers(buffers);\n>>>\n>>> return ret;\n>>> }\n>>> @@ -712,14 +720,23 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n>>> if (data->state_ == RPiCameraData::State::Stopped)\n>>> return -EINVAL;\n>>>\n>>> - /* Ensure all external streams have associated buffers! */\n>>> - for (auto &stream : data->isp_) {\n>>> - if (!stream.isExternal())\n>>> - continue;\n>>> + LOG(RPI, Debug) << \"queueRequestDevice: New request.\";\n>>>\n>>> - if (!request->findBuffer(&stream)) {\n>>> - LOG(RPI, Error) << \"Attempt to queue request with invalid stream.\";\n>>> - return -ENOENT;\n>>> + /* Push all buffers supplied in the Request to the respective streams. */\n>>> + for (auto stream : data->streams_) {\n>>> + if (stream->isExternal()) {\n>>\n>>\n>> Are all streams now 'external' ?\n> \n> All streams but the ISP input (which only imports buffers) *can* be\n> external. They are marked external only if the app configures the\n> pipeline handler by saying it may request a buffer from the stream.\n> In reality, all streams could be marked external all the time, but the\n> buffer handling will take a slightly less efficient path, so I kept\n> the distinction.\n> \n>>\n>>\n>>> + FrameBuffer *buffer = request->findBuffer(stream);\n>>> + /*\n>>> + * A nullptr in buffer means that we should queue an internally\n>>> + * allocated buffer.\n\nNow I understand what's going on here, perhaps this could be expanded a\nbit to explain?\n\n\"If no buffer is provided by the request for this stream, we queue a\nnullptr to the stream to signify that it must use an internally\nallocated buffer for this capture request which will not be given back\nto the application, but can be used to support the internal pipeline flow.\"\n\n\nNow I see how this leads towards zero-copy raw support ;-)\n\n\n\n>>> + *\n>>> + * The below queueBuffer() call will do nothing if there are not\n>>> + * enough internal buffers allocated, but this will be handled by\n>>> + * queuing the request for buffers in the RPiStream object.\n>>> + */\n>>> + int ret = stream->queueBuffer(buffer);\n>>> + if (ret)\n>>> + return ret;\n>>> }\n>>> }\n>>>\n>>> @@ -808,12 +825,10 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n>>> /* Initialize the camera properties. */\n>>> data->properties_ = data->sensor_->properties();\n>>>\n>>> - /*\n>>> - * List the available output streams.\n>>> - * Currently cannot do Unicam streams!\n>>> - */\n>>> + /*List the available streams an application may request. */\n>>\n>> Minor nit, there's a space missing between /* and List ...\n>>\n>>\n>>> std::set streams;\n>>> - streams.insert(&data->isp_[Isp::Input]);\n>>> + streams.insert(&data->unicam_[Unicam::Image]);\n>>> + streams.insert(&data->unicam_[Unicam::Embedded]);\n>>> streams.insert(&data->isp_[Isp::Output0]);\n>>> streams.insert(&data->isp_[Isp::Output1]);\n>>> streams.insert(&data->isp_[Isp::Stats]);\n>>> @@ -831,9 +846,28 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n>>> int ret;\n>>>\n>>> for (auto const stream : data->streams_) {\n>>> - ret = stream->queueBuffers();\n>>> - if (ret < 0)\n>>> - return ret;\n>>> + if (!stream->isExternal()) {\n>>> + ret = stream->queueAllBuffers();\n>>> + if (ret < 0)\n>>> + return ret;\n>>> + } else {\n>>> + /*\n>>> + * For external streams, we must queue up a set of internal\n>>> + * buffers to handle the number of drop frames requested by\n>>> + * the IPA. This is done by passing nullptr in queueBuffer().\n>>> + *\n>>> + * The below queueBuffer() call will do nothing if there\n>>> + * are not enough internal buffers allocated, but this will\n>>> + * be handled by queuing the request for buffers in the\n>>> + * RPiStream object.\n>>> + */\n>>> + unsigned int i;\n>>> + for (i = 0; i < data->dropFrameCount_; i++) {\n>>> + int ret = stream->queueBuffer(nullptr);\n>>> + if (ret)\n>>> + return ret;\n>>> + }\n>>> + }\n>>> }\n>>>\n>>> return 0;\n>>> @@ -847,7 +881,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n>>> /*\n>>> * Decide how many internal buffers to allocate. For now, simply look\n>>> * at how many external buffers will be provided. Will need to improve\n>>> - * this logic.\n>>> + * this logic. However, we really must have all stream allocate the same\n>>\n>> s/stream/streams/\n>>\n>>> + * number of buffers to simplify error handling in queueRequestDevice().\n>>> */\n>>\n>> Does this include Raw streams? I thought that allocates less buffers? Or\n>> perhaps it's the same number of internal buffers, and only 2 extra\n>> external buffers for that case...\n> \n> That's correct. For now all (including Raw) streams allocate the same\n> number of internal buffers. Hence the comment that we could improve\n> this logic, it is a bit wasteful on memory. This bit does need\n> refinement at some point.\n> \n>>\n>>\n>>> unsigned int maxBuffers = 0;\n>>> for (const Stream *s : camera->streams())\n>>> @@ -855,33 +890,9 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n>>> maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);\n>>>\n>>> for (auto const stream : data->streams_) {\n>>> - if (stream->isExternal() || stream->isImporter()) {\n>>> - /*\n>>> - * If a stream is marked as external reserve memory to\n>>> - * prepare to import as many buffers are requested in\n>>> - * the stream configuration.\n>>> - *\n>>> - * If a stream is an internal stream with importer\n>>> - * role, reserve as many buffers as possible.\n>>> - */\n>>> - unsigned int count = stream->isExternal()\n>>> - ? stream->configuration().bufferCount\n>>> - : maxBuffers;\n>>> - ret = stream->importBuffers(count);\n>>> - if (ret < 0)\n>>> - return ret;\n>>> - } else {\n>>> - /*\n>>> - * If the stream is an internal exporter allocate and\n>>> - * export as many buffers as possible to its internal\n>>> - * pool.\n>>> - */\n>>> - ret = stream->allocateBuffers(maxBuffers);\n>>> - if (ret < 0) {\n>>> - freeBuffers(camera);\n>>> - return ret;\n>>> - }\n>>> - }\n>>> + ret = stream->prepareBuffers(maxBuffers);\n>>> + if (ret < 0)\n>>> + return ret;\n>>> }\n>>>\n>>> /*\n>>> @@ -889,7 +900,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n>>> * the IPA and RPI_IPA_EVENT_SIGNAL_ISP_PREPARE event.\n>>> */\n>>> count = 0;\n>>> - for (auto const &b : *data->unicam_[Unicam::Image].getBuffers()) {\n>>> + for (auto const &b : data->unicam_[Unicam::Image].getBuffers()) {\n>>> b->setCookie(count++);\n>>> }\n>>>\n>>> @@ -898,14 +909,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n>>> * the IPA.\n>>> */\n>>> count = 0;\n>>> - for (auto const &b : *data->isp_[Isp::Stats].getBuffers()) {\n>>> + for (auto const &b : data->isp_[Isp::Stats].getBuffers()) {\n>>> b->setCookie(count++);\n>>> data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b->cookie(),\n>>> .planes = b->planes() });\n>>> }\n>>>\n>>> count = 0;\n>>> - for (auto const &b : *data->unicam_[Unicam::Embedded].getBuffers()) {\n>>> + for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {\n>>> b->setCookie(count++);\n>>> data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b->cookie(),\n>>> .planes = b->planes() });\n>>> @@ -1066,7 +1077,7 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n>>> switch (action.operation) {\n>>> case RPI_IPA_ACTION_STATS_METADATA_COMPLETE: {\n>>> unsigned int bufferId = action.data[0];\n>>> - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers()->at(bufferId).get();\n>>> + FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n>>>\n>>> handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n>>> /* Fill the Request metadata buffer with what the IPA has provided */\n>>> @@ -1077,19 +1088,19 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n>>>\n>>> case RPI_IPA_ACTION_EMBEDDED_COMPLETE: {\n>>> unsigned int bufferId = action.data[0];\n>>> - FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers()->at(bufferId).get();\n>>> + FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);\n>>> handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n>>> break;\n>>> }\n>>>\n>>> case RPI_IPA_ACTION_RUN_ISP: {\n>>> unsigned int bufferId = action.data[0];\n>>> - FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers()->at(bufferId).get();\n>>> + FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);\n>>>\n>>> LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << buffer->cookie()\n>>> << \", timestamp: \" << buffer->metadata().timestamp;\n>>>\n>>> - isp_[Isp::Input].dev()->queueBuffer(buffer);\n>>> + isp_[Isp::Input].queueBuffer(buffer);\n>>> ispOutputCount_ = 0;\n>>> break;\n>>> }\n>>> @@ -1190,22 +1201,27 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n>>> << \", buffer id \" << buffer->cookie()\n>>> << \", timestamp: \" << buffer->metadata().timestamp;\n>>>\n>>> - handleStreamBuffer(buffer, stream);\n>>> -\n>>> /*\n>>> - * Increment the number of ISP outputs generated.\n>>> - * This is needed to track dropped frames.\n>>> + * ISP statistics buffer must not be re-queued or sent back to the\n>>> + * application until after the IPA signals so.\n>>> */\n>>> - ispOutputCount_++;\n>>> -\n>>> - /* If this is a stats output, hand it to the IPA now. */\n>>> if (stream == &isp_[Isp::Stats]) {\n>>> IPAOperationData op;\n>>> op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;\n>>> op.data = { RPiIpaMask::STATS | buffer->cookie() };\n>>> ipa_->processEvent(op);\n>>> + } else {\n>>> + /* Any other ISP output can be handed back to the application now. */\n>>> + handleStreamBuffer(buffer, stream);\n>>> }\n>>>\n>>> + /*\n>>> + * Increment the number of ISP outputs generated.\n>>> + * This is needed to track dropped frames.\n>>> + */\n>>> + ispOutputCount_++;\n>>> +\n>>> +\n>>> handleState();\n>>> }\n>>>\n>>> @@ -1218,8 +1234,12 @@ void RPiCameraData::clearIncompleteRequests()\n>>> */\n>>> for (auto const request : requestQueue_) {\n>>> for (auto const stream : streams_) {\n>>> - if (stream->isExternal())\n>>> - stream->dev()->queueBuffer(request->findBuffer(stream));\n>>> + if (!stream->isExternal())\n>>> + continue;\n>>> +\n>>> + FrameBuffer *buffer = request->findBuffer(stream);\n>>> + if (buffer)\n>>> + stream->queueBuffer(buffer);\n>>> }\n>>> }\n>>>\n>>> @@ -1248,7 +1268,7 @@ void RPiCameraData::clearIncompleteRequests()\n>>> * Has the buffer already been handed back to the\n>>> * request? If not, do so now.\n>>> */\n>>> - if (buffer->request())\n>>> + if (buffer && buffer->request())\n>>> pipe_->completeBuffer(camera_, request, buffer);\n>>> }\n>>>\n>>> @@ -1260,30 +1280,24 @@ void RPiCameraData::clearIncompleteRequests()\n>>> void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)\n>>> {\n>>> if (stream->isExternal()) {\n>>> - if (!dropFrameCount_) {\n>>> - Request *request = buffer->request();\n>>> + Request *request = requestQueue_.front();\n>>> +\n>>> + if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {\n>>> + /*\n>>> + * Tag the buffer as completed, returning it to the\n>>> + * application.\n>>> + */\n>>> pipe_->completeBuffer(camera_, request, buffer);\n>>> + } else {\n>>> + /*\n>>> + * This buffer was not part of the Request, so we can\n>>> + * recycle it.\n>>> + */\n>>> + stream->returnBuffer(buffer);\n>>> }\n>>> } else {\n>>> - /* Special handling for RAW buffer Requests.\n>>> - *\n>>> - * The ISP input stream is alway an import stream, but if the\n>>> - * current Request has been made for a buffer on the stream,\n>>> - * simply memcpy to the Request buffer and requeue back to the\n>>> - * device.\n>>> - */\n>>> - if (stream == &unicam_[Unicam::Image] && !dropFrameCount_) {\n>>> - const Stream *rawStream = static_cast(&isp_[Isp::Input]);\n>>> - Request *request = requestQueue_.front();\n>>> - FrameBuffer *raw = request->findBuffer(const_cast(rawStream));\n>>> - if (raw) {\n>>> - raw->copyFrom(buffer);\n>>> - pipe_->completeBuffer(camera_, request, raw);\n>>> - }\n>>> - }\n>>> -\n>>> - /* Simply requeue the buffer. */\n>>> - stream->dev()->queueBuffer(buffer);\n>>> + /* Simply re-queue the buffer to the requested stream. */\n>>> + stream->queueBuffer(buffer);\n>>> }\n>>> }\n>>>\n>>> @@ -1367,7 +1381,7 @@ void RPiCameraData::tryRunPipeline()\n>>> * current bayer buffer will be removed and re-queued to the driver.\n>>> */\n>>> embeddedBuffer = updateQueue(embeddedQueue_, bayerBuffer->metadata().timestamp,\n>>> - unicam_[Unicam::Embedded].dev());\n>>> + &unicam_[Unicam::Embedded]);\n>>>\n>>> if (!embeddedBuffer) {\n>>> LOG(RPI, Debug) << \"Could not find matching embedded buffer\";\n>>> @@ -1386,7 +1400,7 @@ void RPiCameraData::tryRunPipeline()\n>>>\n>>> embeddedBuffer = embeddedQueue_.front();\n>>> bayerBuffer = updateQueue(bayerQueue_, embeddedBuffer->metadata().timestamp,\n>>> - unicam_[Unicam::Image].dev());\n>>> + &unicam_[Unicam::Image]);\n>>>\n>>> if (!bayerBuffer) {\n>>> LOG(RPI, Debug) << \"Could not find matching bayer buffer - ending.\";\n>>> @@ -1394,11 +1408,7 @@ void RPiCameraData::tryRunPipeline()\n>>> }\n>>> }\n>>>\n>>> - /*\n>>> - * Take the first request from the queue and action the IPA.\n>>> - * Unicam buffers for the request have already been queued as they come\n>>> - * in.\n>>> - */\n>>> + /* Take the first request from the queue and action the IPA. */\n>>> Request *request = requestQueue_.front();\n>>>\n>>> /*\n>>> @@ -1410,12 +1420,6 @@ void RPiCameraData::tryRunPipeline()\n>>> op.controls = { request->controls() };\n>>> ipa_->processEvent(op);\n>>>\n>>> - /* Queue up any ISP buffers passed into the request. */\n>>> - for (auto &stream : isp_) {\n>>> - if (stream.isExternal())\n>>> - stream.dev()->queueBuffer(request->findBuffer(&stream));\n>>> - }\n>>> -\n>>> /* Ready to use the buffers, pop them off the queue. */\n>>> bayerQueue_.pop();\n>>> embeddedQueue_.pop();\n>>> @@ -1445,32 +1449,42 @@ void RPiCameraData::tryFlushQueues()\n>>> * and give a chance for the hardware to return to lock-step. We do have\n>>> * to drop all interim frames.\n>>> */\n>>> - if (unicam_[Unicam::Image].getBuffers()->size() == bayerQueue_.size() &&\n>>> - unicam_[Unicam::Embedded].getBuffers()->size() == embeddedQueue_.size()) {\n>>> + if (unicam_[Unicam::Image].getBuffers().size() == bayerQueue_.size() &&\n>>> + unicam_[Unicam::Embedded].getBuffers().size() == embeddedQueue_.size()) {\n>>> + /* This cannot happen when Unicam streams are external. */\n>>> + assert(!unicam_[Unicam::Image].isExternal());\n>>> +\n>>> LOG(RPI, Warning) << \"Flushing all buffer queues!\";\n>>>\n>>> while (!bayerQueue_.empty()) {\n>>> - unicam_[Unicam::Image].dev()->queueBuffer(bayerQueue_.front());\n>>> + unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());\n>>> bayerQueue_.pop();\n>>> }\n>>>\n>>> while (!embeddedQueue_.empty()) {\n>>> - unicam_[Unicam::Embedded].dev()->queueBuffer(embeddedQueue_.front());\n>>> + unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());\n>>> embeddedQueue_.pop();\n>>> }\n>>> }\n>>> }\n>>>\n>>> FrameBuffer *RPiCameraData::updateQueue(std::queue &q, uint64_t timestamp,\n>>> - V4L2VideoDevice *dev)\n>>> + RPi::RPiStream *stream)\n>>> {\n>>> + /*\n>>> + * If the unicam streams are external (both have to the same), then we\n>>\n>> both have to 'be' the same?\n>>\n>>> + * can only return out the top buffer in the queue, and assume they have\n>>> + * been synced by queuing at the same time. We cannot drop these frames,\n>>> + * as they may have been provided externally.\n>>> + */\n>>> while (!q.empty()) {\n>>> FrameBuffer *b = q.front();\n>>> - if (b->metadata().timestamp < timestamp) {\n>>> + if (!stream->isExternal() && b->metadata().timestamp < timestamp) {\n>>> q.pop();\n>>> - dev->queueBuffer(b);\n>>> - LOG(RPI, Warning) << \"Dropping input frame!\";\n>>> - } else if (b->metadata().timestamp == timestamp) {\n>>> + stream->queueBuffer(b);\n>>> + LOG(RPI, Warning) << \"Dropping unmatched input frame in stream \"\n>>> + << stream->name();\n>>> + } else if (stream->isExternal() || b->metadata().timestamp == timestamp) {\n>>> /* The calling function will pop the item from the queue. */\n>>> return b;\n>>> } else {\n>>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n>>> index 2edb8b59..02f8d3e0 100644\n>>> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n>>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n>>> @@ -21,30 +21,20 @@ V4L2VideoDevice *RPiStream::dev() const\n>>>\n>>> void RPiStream::setExternal(bool external)\n>>> {\n>>> + /* Import streams cannot be external. */\n>>> + assert(!external || !importOnly_);\n>>> external_ = external;\n>>> }\n>>>\n>>> bool RPiStream::isExternal() const\n>>> {\n>>> - /*\n>>> - * Import streams cannot be external.\n>>> - *\n>>> - * RAW capture is a special case where we simply copy the RAW\n>>> - * buffer out of the request. All other buffer handling happens\n>>> - * as if the stream is internal.\n>>> - */\n>>> - return external_ && !importOnly_;\n>>> -}\n>>> -\n>>> -bool RPiStream::isImporter() const\n>>> -{\n>>> - return importOnly_;\n>>> + return external_;\n>>> }\n>>>\n>>> void RPiStream::reset()\n>>> {\n>>> external_ = false;\n>>> - internalBuffers_.clear();\n>>> + clearBuffers();\n>>> }\n>>>\n>>> std::string RPiStream::name() const\n>>> @@ -52,65 +42,123 @@ std::string RPiStream::name() const\n>>> return name_;\n>>> }\n>>>\n>>> -void RPiStream::setExternalBuffers(std::vector> *buffers)\n>>> +void RPiStream::setExportedBuffers(std::vector> *buffers)\n>>> {\n>>> - externalBuffers_ = buffers;\n>>> + std::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_),\n>>> + [](std::unique_ptr &b) { return b.get(); });\n>>> }\n>>>\n>>> -const std::vector> *RPiStream::getBuffers() const\n>>> +const std::vector &RPiStream::getBuffers() const\n>>> {\n>>> - return external_ ? externalBuffers_ : &internalBuffers_;\n>>> + return bufferList_;\n>>> }\n>>>\n>>> void RPiStream::releaseBuffers()\n>>> {\n>>> dev_->releaseBuffers();\n>>> - if (!external_ && !importOnly_)\n>>> - internalBuffers_.clear();\n>>> + clearBuffers();\n>>> }\n>>>\n>>> -int RPiStream::importBuffers(unsigned int count)\n>>> +int RPiStream::prepareBuffers(unsigned int count)\n>>> {\n>>> + int ret;\n>>> +\n>>> + if (!importOnly_) {\n>>> + if (count) {\n>>> + /* Export some frame buffers for internal use. */\n>>> + ret = dev_->exportBuffers(count, &internalBuffers_);\n>>> + if (ret < 0)\n>>> + return ret;\n>>> +\n>>> + /* Add these exported buffers to the internal/external buffer list. */\n>>> + setExportedBuffers(&internalBuffers_);\n>>> +\n>>> + /* Add these buffers to the queue of internal usable buffers. */\n>>> + for (auto const &buffer : internalBuffers_)\n>>> + availableBuffers_.push(buffer.get());\n>>> + }\n>>> +\n>>> + /* We must import all internal/external exported buffers. */\n>>> + count = bufferList_.size();\n>>> + }\n>>> +\n>>> return dev_->importBuffers(count);\n>>> }\n>>>\n>>> -int RPiStream::allocateBuffers(unsigned int count)\n>>> +int RPiStream::queueAllBuffers()\n>>> {\n>>> - return dev_->allocateBuffers(count, &internalBuffers_);\n>>> -}\n>>> + int ret;\n>>>\n>>> -int RPiStream::queueBuffers()\n>>> -{\n>>> if (external_)\n>>> return 0;\n>>>\n>>> - for (auto &b : internalBuffers_) {\n>>> - int ret = dev_->queueBuffer(b.get());\n>>> - if (ret) {\n>>> - LOG(RPISTREAM, Error) << \"Failed to queue buffers for \"\n>>> - << name_;\n>>> + while (!availableBuffers_.empty()) {\n>>> + ret = queueBuffer(availableBuffers_.front());\n>>> + if (ret < 0)\n>>> return ret;\n>>> - }\n>>> +\n>>> + availableBuffers_.pop();\n>>> }\n>>>\n>>> return 0;\n>>> }\n>>>\n>>> -bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const\n>>> +int RPiStream::queueBuffer(FrameBuffer *buffer)\n>>> +{\n>>> + /*\n>>> + * A nullptr buffer implies an external stream, but no external\n>>> + * buffer has been supplied. So, pick one from the availableBuffers_\n>>> + * queue.\n\nAha, now I see this here, maybe the comment addition I suggested above\nis possibly redundant, as it's explained here ...\n\n\n\n>>> + */\n>>> + if (!buffer) {\n>>> + if (availableBuffers_.empty()) {\n>>> + LOG(RPISTREAM, Warning) << \"No buffers available for \"\n>>> + << name_;\n>>> + return -EINVAL;\n>>> + }\n>>> +\n>>> + buffer = availableBuffers_.front();\n>>> + availableBuffers_.pop();\n>>> + }\n>>> +\n>>> + LOG(RPISTREAM, Debug) << \"Queuing buffer \" << buffer->cookie()\n>>> + << \" for \" << name_;\n>>> +\n>>> + int ret = dev_->queueBuffer(buffer);\n>>> + if (ret) {\n>>> + LOG(RPISTREAM, Error) << \"Failed to queue buffer for \"\n>>> + << name_;\n>>> + }\n>>> +\n>>> + return ret;\n>>> +}\n>>> +\n>>> +void RPiStream::returnBuffer(FrameBuffer *buffer)\n>>> {\n>>> - auto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();\n>>> - auto end = external_ ? externalBuffers_->end() : internalBuffers_.end();\n>>> + /* This can only be called for external streams. */\n>>> + assert(external_);\n\nCould this use ASSERT() from libcamera/internal/log.h ?\n\n>>> +\n>>> + availableBuffers_.push(buffer);\n>>> +}\n>>>\n>>> +bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const\n>>> +{\n>>> if (importOnly_)\n>>> return false;\n>>>\n>>> - if (std::find_if(start, end,\n>>> - [buffer](std::unique_ptr const &ref) { return ref.get() == buffer; }) != end)\n>>> + if (std::find(bufferList_.begin(), bufferList_.end(), buffer) != bufferList_.end())\n>>> return true;\n>>>\n>>> return false;\n>>> }\n>>>\n>>> +void RPiStream::clearBuffers()\n>>> +{\n>>> + availableBuffers_ = std::queue{};\n>>> + internalBuffers_.clear();\n>>> + bufferList_.clear();\n>>> +}\n>>> +\n>>> } /* namespace RPi */\n>>>\n>>> } /* namespace libcamera */\n>>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n>>> index 3957e342..af9c2ad2 100644\n>>> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n>>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n>>> @@ -39,21 +39,22 @@ public:\n>>> V4L2VideoDevice *dev() const;\n>>> void setExternal(bool external);\n>>> bool isExternal() const;\n>>> - bool isImporter() const;\n>>> void reset();\n>>> std::string name() const;\n>>> - void setExternalBuffers(std::vector> *buffers);\n>>> - const std::vector> *getBuffers() const;\n>>> + void setExportedBuffers(std::vector> *buffers);\n>>> + const std::vector &getBuffers() const;\n>>> void releaseBuffers();\n>>> - int importBuffers(unsigned int count);\n>>> - int allocateBuffers(unsigned int count);\n>>> - int queueBuffers();\n>>> + int prepareBuffers(unsigned int count);\n>>> + int queueAllBuffers();\n>>> + int queueBuffer(FrameBuffer *buffer);\n>>> + void returnBuffer(FrameBuffer *buffer);\n>>> bool findFrameBuffer(FrameBuffer *buffer) const;\n>>\n>>\n>> Perhaps we should break those up into sections in patch 1/9. It's hard\n>> to see the wood-for-the-trees in the wall of function declarations.\n> \n> Not sure I follow what you mean?\n\n\nThere is a lot of text all bunched up together, so it's hard to get my\neyes around the class members.\n\nIt would be nice to group functions with some line breaks to make it\neasier on the eye.\n\nBut that should happen in patch 1/9, so I'll go comment on it there. I\nskipped it before because that patch just moves code around, but I thnik\nit can also improve formatting for readability.\n\n\n\n> \n>>\n>>\n>>>\n>>> private:\n>>> + void clearBuffers();\n>>> /*\n>>> * Indicates that this stream is active externally, i.e. the buffers\n>>> - * are provided by the application.\n>>> + * might be provided by (and returned to) the application.\n>>> */\n>>> bool external_;\n>>> /* Indicates that this stream only imports buffers, e.g. ISP input. */\n>>> @@ -62,10 +63,19 @@ private:\n>>> std::string name_;\n>>> /* The actual device stream. */\n>>> std::unique_ptr dev_;\n>>> - /* Internally allocated framebuffers associated with this device stream. */\n>>> + /* All framebuffers associated with this device stream. */\n>>> + std::vector bufferList_;\n>>> + /*\n>>> + * List of frame buffer that we can use if none have been provided by\n>>> + * the application for external streams. This is populated by the\n>>> + * buffers exported internally.\n>>> + */\n>>> + std::queue availableBuffers_;\n>>> + /*\n>>> + * This is a list of buffers exported internally. Need to keep this around\n>>> + * as the stream needs to maintain ownership of these buffers.\n>>> + */\n>>> std::vector> internalBuffers_;\n>>> - /* Externally allocated framebuffers associated with this device stream. */\n>>> - std::vector> *externalBuffers_;\n>>\n>> Likewise here, some whitespace to break up these members would really\n>> help, as by the time I've got here I've got definite review fatigue and\n>> lots of lines close together just become a blur ... :S\n> \n> That's not a problem.\n\n\nBut for this patch, I can't see anything wrong ... and I can see how\nit's helping manage buffers to allow optional buffers on a stream for\nRAW zero copy support.\n\nReviewed-by: Kieran Bingham \n\n\n> \n>>\n>>\n>>> };\n>>>\n>>> /*\n>>>\n>>\n>> --\n>> Regards\n>> --\n>> Kieran\n> \n> Regards,\n> Naush\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 7EBD8C2E68\n\tfor ;\n\tWed, 22 Jul 2020 12:21:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0F15060943;\n\tWed, 22 Jul 2020 14:21:38 +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 EED456039F\n\tfor ;\n\tWed, 22 Jul 2020 14:21:36 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2301F329;\n\tWed, 22 Jul 2020 14:21:36 +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=\"wWs1pDQA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1595420496;\n\tbh=T9hnsgIDMWVTGvzmdnpYIo8R3xV6IRYdrvZTts4srrM=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=wWs1pDQAEKMUinALb0RM2hoWI2uNRvSM3nX3iz0XPOvVyfcB2qbRdxgJXNCdu3xQ2\n\t1Hrl6iGPRyV9MGSsrB1673/zVQ+qDlOfYF2qmdltinb/WPpEuKhqwPZ66et2LpadUr\n\tMrIR2nchV/5ORckFP2Bp0Z2UlbZJ4eJNcGc/UbKk=","To":"Naushir Patuck ","References":"<20200720091311.805092-1-naush@raspberrypi.com>\n\t<20200720091311.805092-7-naush@raspberrypi.com>\n\t<4577acb0-3976-4721-6673-e456db4dc4b3@ideasonboard.com>\n\t","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<2a259e2a-9cb6-73c3-26f6-328a78077b48@ideasonboard.com>","Date":"Wed, 22 Jul 2020 13:21:33 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v4 6/9] libcamera: pipeline:\n\traspberrypi: Rework stream buffer logic for zero-copy","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11506,"web_url":"https://patchwork.libcamera.org/comment/11506/","msgid":"<20200722152513.GH29813@pendragon.ideasonboard.com>","date":"2020-07-22T15:25:13","subject":"Re: [libcamera-devel] [PATCH v4 6/9] libcamera: pipeline:\n\traspberrypi: Rework stream buffer logic for zero-copy","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Wed, Jul 22, 2020 at 01:21:33PM +0100, Kieran Bingham wrote:\n> On 22/07/2020 09:04, Naushir Patuck wrote:\n> > On Tue, 21 Jul 2020 at 16:41, Kieran Bingham wrote:\n> >> On 20/07/2020 10:13, Naushir Patuck wrote:\n> >>> Stop using v4l2_videodevice::allocateBuffer() for internal buffers and\n> >>> instead export/import all buffers. This allows the pipeline to return\n> >>> any stream buffer requested by the application as zero-copy.\n> >>>\n> >>> Advertise the Unicam Image stream as the RAW capture stream now.\n> >>>\n> >>> The RPiStream object now maintains a new list of buffers that are\n> >>> available to queue into a device. This is needed to distinguish between\n> >>> FrameBuffers allocated for internal use vs externally provided buffers.\n> >>> When a Request comes in, if a buffer is not provided for an exported\n> >>> stream, we re-use a buffer from this list.\n> >>>\n> >>> Signed-off-by: Naushir Patuck \n> >>> Reviewed-by: Niklas Söderlund \n> >>\n> >> A few small nit fixups below but they could possibly be fixed while\n> >> applying anyway if you prefer.\n> >>\n> >> I've hit review fatigue though, so I'll look at this again tomorrow.\n> > \n> > I understand :)\n> > I will make the necessary fixups based on your review comments, and\n> > submit a new patch set. Will wait until you have finished reviewing\n> > this one before I send the next revision.\n> \n> Looking good, I think the only actionable thing remaining here is\n> potentially to use ASSERT() over assert() :-)\n> \n> >>> ---\n> >>> .../pipeline/raspberrypi/raspberrypi.cpp | 228 ++++++++++--------\n> >>> .../pipeline/raspberrypi/rpi_stream.cpp | 122 +++++++---\n> >>> .../pipeline/raspberrypi/rpi_stream.h | 30 ++-\n> >>> 3 files changed, 226 insertions(+), 154 deletions(-)\n> >>>\n> >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>> index 8f6a999b..dbc22521 100644\n> >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>> @@ -187,7 +187,8 @@ private:\n> >>> void checkRequestCompleted();\n> >>> void tryRunPipeline();\n> >>> void tryFlushQueues();\n> >>> - FrameBuffer *updateQueue(std::queue &q, uint64_t timestamp, V4L2VideoDevice *dev);\n> >>> + FrameBuffer *updateQueue(std::queue &q, uint64_t timestamp,\n> >>> + RPi::RPiStream *stream);\n> >>>\n> >>> unsigned int ispOutputCount_;\n> >>> };\n> >>> @@ -508,8 +509,15 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> >>> StreamConfiguration &cfg = config->at(i);\n> >>>\n> >>> if (isRaw(cfg.pixelFormat)) {\n> >>> - cfg.setStream(&data->isp_[Isp::Input]);\n> >>> - data->isp_[Isp::Input].setExternal(true);\n> >>> + cfg.setStream(&data->unicam_[Unicam::Image]);\n> >>> + /*\n> >>> + * We must set both Unicam streams as external, even\n> >>> + * though the application may only request RAW frames.\n> >>> + * This is because we match timestamps on both streams\n> >>> + * to synchronise buffers.\n> >>> + */\n> >>> + data->unicam_[Unicam::Image].setExternal(true);\n> >>> + data->unicam_[Unicam::Embedded].setExternal(true);\n> >>> continue;\n> >>> }\n> >>>\n> >>> @@ -612,7 +620,7 @@ int PipelineHandlerRPi::exportFrameBuffers(Camera *camera, Stream *stream,\n> >>> unsigned int count = stream->configuration().bufferCount;\n> >>> int ret = s->dev()->exportBuffers(count, buffers);\n> >>>\n> >>> - s->setExternalBuffers(buffers);\n> >>> + s->setExportedBuffers(buffers);\n> >>>\n> >>> return ret;\n> >>> }\n> >>> @@ -712,14 +720,23 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> >>> if (data->state_ == RPiCameraData::State::Stopped)\n> >>> return -EINVAL;\n> >>>\n> >>> - /* Ensure all external streams have associated buffers! */\n> >>> - for (auto &stream : data->isp_) {\n> >>> - if (!stream.isExternal())\n> >>> - continue;\n> >>> + LOG(RPI, Debug) << \"queueRequestDevice: New request.\";\n> >>>\n> >>> - if (!request->findBuffer(&stream)) {\n> >>> - LOG(RPI, Error) << \"Attempt to queue request with invalid stream.\";\n> >>> - return -ENOENT;\n> >>> + /* Push all buffers supplied in the Request to the respective streams. */\n> >>> + for (auto stream : data->streams_) {\n> >>> + if (stream->isExternal()) {\n> >>\n> >> Are all streams now 'external' ?\n> > \n> > All streams but the ISP input (which only imports buffers) *can* be\n> > external. They are marked external only if the app configures the\n> > pipeline handler by saying it may request a buffer from the stream.\n> > In reality, all streams could be marked external all the time, but the\n> > buffer handling will take a slightly less efficient path, so I kept\n> > the distinction.\n> > \n> >>> + FrameBuffer *buffer = request->findBuffer(stream);\n> >>> + /*\n> >>> + * A nullptr in buffer means that we should queue an internally\n> >>> + * allocated buffer.\n> \n> Now I understand what's going on here, perhaps this could be expanded a\n> bit to explain?\n> \n> \"If no buffer is provided by the request for this stream, we queue a\n> nullptr to the stream to signify that it must use an internally\n> allocated buffer for this capture request which will not be given back\n> to the application, but can be used to support the internal pipeline flow.\"\n>\n> Now I see how this leads towards zero-copy raw support ;-)\n> \n> >>> + *\n> >>> + * The below queueBuffer() call will do nothing if there are not\n> >>> + * enough internal buffers allocated, but this will be handled by\n> >>> + * queuing the request for buffers in the RPiStream object.\n> >>> + */\n> >>> + int ret = stream->queueBuffer(buffer);\n> >>> + if (ret)\n> >>> + return ret;\n> >>> }\n> >>> }\n> >>>\n> >>> @@ -808,12 +825,10 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> >>> /* Initialize the camera properties. */\n> >>> data->properties_ = data->sensor_->properties();\n> >>>\n> >>> - /*\n> >>> - * List the available output streams.\n> >>> - * Currently cannot do Unicam streams!\n> >>> - */\n> >>> + /*List the available streams an application may request. */\n> >>\n> >> Minor nit, there's a space missing between /* and List ...\n> >>\n> >>> std::set streams;\n> >>> - streams.insert(&data->isp_[Isp::Input]);\n> >>> + streams.insert(&data->unicam_[Unicam::Image]);\n> >>> + streams.insert(&data->unicam_[Unicam::Embedded]);\n\nCan an application request the embedded data ?\n\n> >>> streams.insert(&data->isp_[Isp::Output0]);\n> >>> streams.insert(&data->isp_[Isp::Output1]);\n> >>> streams.insert(&data->isp_[Isp::Stats]);\n> >>> @@ -831,9 +846,28 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> >>> int ret;\n> >>>\n> >>> for (auto const stream : data->streams_) {\n> >>> - ret = stream->queueBuffers();\n> >>> - if (ret < 0)\n> >>> - return ret;\n> >>> + if (!stream->isExternal()) {\n> >>> + ret = stream->queueAllBuffers();\n> >>> + if (ret < 0)\n> >>> + return ret;\n> >>> + } else {\n> >>> + /*\n> >>> + * For external streams, we must queue up a set of internal\n> >>> + * buffers to handle the number of drop frames requested by\n> >>> + * the IPA. This is done by passing nullptr in queueBuffer().\n> >>> + *\n> >>> + * The below queueBuffer() call will do nothing if there\n> >>> + * are not enough internal buffers allocated, but this will\n> >>> + * be handled by queuing the request for buffers in the\n> >>> + * RPiStream object.\n> >>> + */\n> >>> + unsigned int i;\n> >>> + for (i = 0; i < data->dropFrameCount_; i++) {\n> >>> + int ret = stream->queueBuffer(nullptr);\n> >>> + if (ret)\n> >>> + return ret;\n> >>> + }\n> >>> + }\n> >>> }\n> >>>\n> >>> return 0;\n> >>> @@ -847,7 +881,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> >>> /*\n> >>> * Decide how many internal buffers to allocate. For now, simply look\n> >>> * at how many external buffers will be provided. Will need to improve\n> >>> - * this logic.\n> >>> + * this logic. However, we really must have all stream allocate the same\n> >>\n> >> s/stream/streams/\n> >>\n> >>> + * number of buffers to simplify error handling in queueRequestDevice().\n> >>> */\n> >>\n> >> Does this include Raw streams? I thought that allocates less buffers? Or\n> >> perhaps it's the same number of internal buffers, and only 2 extra\n> >> external buffers for that case...\n> > \n> > That's correct. For now all (including Raw) streams allocate the same\n> > number of internal buffers. Hence the comment that we could improve\n> > this logic, it is a bit wasteful on memory. This bit does need\n> > refinement at some point.\n> > \n> >>> unsigned int maxBuffers = 0;\n> >>> for (const Stream *s : camera->streams())\n> >>> @@ -855,33 +890,9 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> >>> maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);\n> >>>\n> >>> for (auto const stream : data->streams_) {\n> >>> - if (stream->isExternal() || stream->isImporter()) {\n> >>> - /*\n> >>> - * If a stream is marked as external reserve memory to\n> >>> - * prepare to import as many buffers are requested in\n> >>> - * the stream configuration.\n> >>> - *\n> >>> - * If a stream is an internal stream with importer\n> >>> - * role, reserve as many buffers as possible.\n> >>> - */\n> >>> - unsigned int count = stream->isExternal()\n> >>> - ? stream->configuration().bufferCount\n> >>> - : maxBuffers;\n> >>> - ret = stream->importBuffers(count);\n> >>> - if (ret < 0)\n> >>> - return ret;\n> >>> - } else {\n> >>> - /*\n> >>> - * If the stream is an internal exporter allocate and\n> >>> - * export as many buffers as possible to its internal\n> >>> - * pool.\n> >>> - */\n> >>> - ret = stream->allocateBuffers(maxBuffers);\n> >>> - if (ret < 0) {\n> >>> - freeBuffers(camera);\n> >>> - return ret;\n> >>> - }\n> >>> - }\n> >>> + ret = stream->prepareBuffers(maxBuffers);\n> >>> + if (ret < 0)\n> >>> + return ret;\n\nThis doesn't free buffers anymore on failure, is that an oversight ?\n\n> >>> }\n> >>>\n> >>> /*\n> >>> @@ -889,7 +900,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> >>> * the IPA and RPI_IPA_EVENT_SIGNAL_ISP_PREPARE event.\n> >>> */\n> >>> count = 0;\n> >>> - for (auto const &b : *data->unicam_[Unicam::Image].getBuffers()) {\n> >>> + for (auto const &b : data->unicam_[Unicam::Image].getBuffers()) {\n> >>> b->setCookie(count++);\n> >>> }\n> >>>\n> >>> @@ -898,14 +909,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> >>> * the IPA.\n> >>> */\n> >>> count = 0;\n> >>> - for (auto const &b : *data->isp_[Isp::Stats].getBuffers()) {\n> >>> + for (auto const &b : data->isp_[Isp::Stats].getBuffers()) {\n> >>> b->setCookie(count++);\n> >>> data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b->cookie(),\n> >>> .planes = b->planes() });\n> >>> }\n> >>>\n> >>> count = 0;\n> >>> - for (auto const &b : *data->unicam_[Unicam::Embedded].getBuffers()) {\n> >>> + for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {\n> >>> b->setCookie(count++);\n> >>> data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b->cookie(),\n> >>> .planes = b->planes() });\n> >>> @@ -1066,7 +1077,7 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n> >>> switch (action.operation) {\n> >>> case RPI_IPA_ACTION_STATS_METADATA_COMPLETE: {\n> >>> unsigned int bufferId = action.data[0];\n> >>> - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers()->at(bufferId).get();\n> >>> + FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> >>>\n> >>> handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> >>> /* Fill the Request metadata buffer with what the IPA has provided */\n> >>> @@ -1077,19 +1088,19 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n> >>>\n> >>> case RPI_IPA_ACTION_EMBEDDED_COMPLETE: {\n> >>> unsigned int bufferId = action.data[0];\n> >>> - FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers()->at(bufferId).get();\n> >>> + FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);\n> >>> handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> >>> break;\n> >>> }\n> >>>\n> >>> case RPI_IPA_ACTION_RUN_ISP: {\n> >>> unsigned int bufferId = action.data[0];\n> >>> - FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers()->at(bufferId).get();\n> >>> + FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);\n> >>>\n> >>> LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << buffer->cookie()\n> >>> << \", timestamp: \" << buffer->metadata().timestamp;\n> >>>\n> >>> - isp_[Isp::Input].dev()->queueBuffer(buffer);\n> >>> + isp_[Isp::Input].queueBuffer(buffer);\n> >>> ispOutputCount_ = 0;\n> >>> break;\n> >>> }\n> >>> @@ -1190,22 +1201,27 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n> >>> << \", buffer id \" << buffer->cookie()\n> >>> << \", timestamp: \" << buffer->metadata().timestamp;\n> >>>\n> >>> - handleStreamBuffer(buffer, stream);\n> >>> -\n> >>> /*\n> >>> - * Increment the number of ISP outputs generated.\n> >>> - * This is needed to track dropped frames.\n> >>> + * ISP statistics buffer must not be re-queued or sent back to the\n> >>> + * application until after the IPA signals so.\n> >>> */\n> >>> - ispOutputCount_++;\n> >>> -\n> >>> - /* If this is a stats output, hand it to the IPA now. */\n> >>> if (stream == &isp_[Isp::Stats]) {\n> >>> IPAOperationData op;\n> >>> op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;\n> >>> op.data = { RPiIpaMask::STATS | buffer->cookie() };\n> >>> ipa_->processEvent(op);\n> >>> + } else {\n> >>> + /* Any other ISP output can be handed back to the application now. */\n> >>> + handleStreamBuffer(buffer, stream);\n> >>> }\n> >>>\n> >>> + /*\n> >>> + * Increment the number of ISP outputs generated.\n> >>> + * This is needed to track dropped frames.\n> >>> + */\n> >>> + ispOutputCount_++;\n> >>> +\n> >>> +\n\nExtra blank line ?\n\n> >>> handleState();\n> >>> }\n> >>>\n> >>> @@ -1218,8 +1234,12 @@ void RPiCameraData::clearIncompleteRequests()\n> >>> */\n> >>> for (auto const request : requestQueue_) {\n> >>> for (auto const stream : streams_) {\n> >>> - if (stream->isExternal())\n> >>> - stream->dev()->queueBuffer(request->findBuffer(stream));\n> >>> + if (!stream->isExternal())\n> >>> + continue;\n> >>> +\n> >>> + FrameBuffer *buffer = request->findBuffer(stream);\n> >>> + if (buffer)\n> >>> + stream->queueBuffer(buffer);\n> >>> }\n> >>> }\n> >>>\n> >>> @@ -1248,7 +1268,7 @@ void RPiCameraData::clearIncompleteRequests()\n> >>> * Has the buffer already been handed back to the\n> >>> * request? If not, do so now.\n> >>> */\n> >>> - if (buffer->request())\n> >>> + if (buffer && buffer->request())\n> >>> pipe_->completeBuffer(camera_, request, buffer);\n> >>> }\n> >>>\n> >>> @@ -1260,30 +1280,24 @@ void RPiCameraData::clearIncompleteRequests()\n> >>> void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)\n> >>> {\n> >>> if (stream->isExternal()) {\n> >>> - if (!dropFrameCount_) {\n> >>> - Request *request = buffer->request();\n> >>> + Request *request = requestQueue_.front();\n> >>> +\n> >>> + if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {\n> >>> + /*\n> >>> + * Tag the buffer as completed, returning it to the\n> >>> + * application.\n> >>> + */\n> >>> pipe_->completeBuffer(camera_, request, buffer);\n> >>> + } else {\n> >>> + /*\n> >>> + * This buffer was not part of the Request, so we can\n> >>> + * recycle it.\n> >>> + */\n> >>> + stream->returnBuffer(buffer);\n> >>> }\n> >>> } else {\n> >>> - /* Special handling for RAW buffer Requests.\n> >>> - *\n> >>> - * The ISP input stream is alway an import stream, but if the\n> >>> - * current Request has been made for a buffer on the stream,\n> >>> - * simply memcpy to the Request buffer and requeue back to the\n> >>> - * device.\n> >>> - */\n> >>> - if (stream == &unicam_[Unicam::Image] && !dropFrameCount_) {\n> >>> - const Stream *rawStream = static_cast(&isp_[Isp::Input]);\n> >>> - Request *request = requestQueue_.front();\n> >>> - FrameBuffer *raw = request->findBuffer(const_cast(rawStream));\n> >>> - if (raw) {\n> >>> - raw->copyFrom(buffer);\n> >>> - pipe_->completeBuffer(camera_, request, raw);\n> >>> - }\n> >>> - }\n> >>> -\n> >>> - /* Simply requeue the buffer. */\n> >>> - stream->dev()->queueBuffer(buffer);\n> >>> + /* Simply re-queue the buffer to the requested stream. */\n> >>> + stream->queueBuffer(buffer);\n> >>> }\n> >>> }\n> >>>\n> >>> @@ -1367,7 +1381,7 @@ void RPiCameraData::tryRunPipeline()\n> >>> * current bayer buffer will be removed and re-queued to the driver.\n> >>> */\n> >>> embeddedBuffer = updateQueue(embeddedQueue_, bayerBuffer->metadata().timestamp,\n> >>> - unicam_[Unicam::Embedded].dev());\n> >>> + &unicam_[Unicam::Embedded]);\n> >>>\n> >>> if (!embeddedBuffer) {\n> >>> LOG(RPI, Debug) << \"Could not find matching embedded buffer\";\n> >>> @@ -1386,7 +1400,7 @@ void RPiCameraData::tryRunPipeline()\n> >>>\n> >>> embeddedBuffer = embeddedQueue_.front();\n> >>> bayerBuffer = updateQueue(bayerQueue_, embeddedBuffer->metadata().timestamp,\n> >>> - unicam_[Unicam::Image].dev());\n> >>> + &unicam_[Unicam::Image]);\n> >>>\n> >>> if (!bayerBuffer) {\n> >>> LOG(RPI, Debug) << \"Could not find matching bayer buffer - ending.\";\n> >>> @@ -1394,11 +1408,7 @@ void RPiCameraData::tryRunPipeline()\n> >>> }\n> >>> }\n> >>>\n> >>> - /*\n> >>> - * Take the first request from the queue and action the IPA.\n> >>> - * Unicam buffers for the request have already been queued as they come\n> >>> - * in.\n> >>> - */\n> >>> + /* Take the first request from the queue and action the IPA. */\n> >>> Request *request = requestQueue_.front();\n> >>>\n> >>> /*\n> >>> @@ -1410,12 +1420,6 @@ void RPiCameraData::tryRunPipeline()\n> >>> op.controls = { request->controls() };\n> >>> ipa_->processEvent(op);\n> >>>\n> >>> - /* Queue up any ISP buffers passed into the request. */\n> >>> - for (auto &stream : isp_) {\n> >>> - if (stream.isExternal())\n> >>> - stream.dev()->queueBuffer(request->findBuffer(&stream));\n> >>> - }\n> >>> -\n> >>> /* Ready to use the buffers, pop them off the queue. */\n> >>> bayerQueue_.pop();\n> >>> embeddedQueue_.pop();\n> >>> @@ -1445,32 +1449,42 @@ void RPiCameraData::tryFlushQueues()\n> >>> * and give a chance for the hardware to return to lock-step. We do have\n> >>> * to drop all interim frames.\n> >>> */\n> >>> - if (unicam_[Unicam::Image].getBuffers()->size() == bayerQueue_.size() &&\n> >>> - unicam_[Unicam::Embedded].getBuffers()->size() == embeddedQueue_.size()) {\n> >>> + if (unicam_[Unicam::Image].getBuffers().size() == bayerQueue_.size() &&\n> >>> + unicam_[Unicam::Embedded].getBuffers().size() == embeddedQueue_.size()) {\n> >>> + /* This cannot happen when Unicam streams are external. */\n> >>> + assert(!unicam_[Unicam::Image].isExternal());\n> >>> +\n> >>> LOG(RPI, Warning) << \"Flushing all buffer queues!\";\n> >>>\n> >>> while (!bayerQueue_.empty()) {\n> >>> - unicam_[Unicam::Image].dev()->queueBuffer(bayerQueue_.front());\n> >>> + unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());\n> >>> bayerQueue_.pop();\n> >>> }\n> >>>\n> >>> while (!embeddedQueue_.empty()) {\n> >>> - unicam_[Unicam::Embedded].dev()->queueBuffer(embeddedQueue_.front());\n> >>> + unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());\n> >>> embeddedQueue_.pop();\n> >>> }\n> >>> }\n> >>> }\n> >>>\n> >>> FrameBuffer *RPiCameraData::updateQueue(std::queue &q, uint64_t timestamp,\n> >>> - V4L2VideoDevice *dev)\n> >>> + RPi::RPiStream *stream)\n> >>> {\n> >>> + /*\n> >>> + * If the unicam streams are external (both have to the same), then we\n> >>\n> >> both have to 'be' the same?\n> >>\n> >>> + * can only return out the top buffer in the queue, and assume they have\n> >>> + * been synced by queuing at the same time. We cannot drop these frames,\n> >>> + * as they may have been provided externally.\n> >>> + */\n> >>> while (!q.empty()) {\n> >>> FrameBuffer *b = q.front();\n> >>> - if (b->metadata().timestamp < timestamp) {\n> >>> + if (!stream->isExternal() && b->metadata().timestamp < timestamp) {\n> >>> q.pop();\n> >>> - dev->queueBuffer(b);\n> >>> - LOG(RPI, Warning) << \"Dropping input frame!\";\n> >>> - } else if (b->metadata().timestamp == timestamp) {\n> >>> + stream->queueBuffer(b);\n> >>> + LOG(RPI, Warning) << \"Dropping unmatched input frame in stream \"\n> >>> + << stream->name();\n> >>> + } else if (stream->isExternal() || b->metadata().timestamp == timestamp) {\n> >>> /* The calling function will pop the item from the queue. */\n> >>> return b;\n> >>> } else {\n> >>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> >>> index 2edb8b59..02f8d3e0 100644\n> >>> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> >>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> >>> @@ -21,30 +21,20 @@ V4L2VideoDevice *RPiStream::dev() const\n> >>>\n> >>> void RPiStream::setExternal(bool external)\n> >>> {\n> >>> + /* Import streams cannot be external. */\n> >>> + assert(!external || !importOnly_);\n> >>> external_ = external;\n> >>> }\n> >>>\n> >>> bool RPiStream::isExternal() const\n> >>> {\n> >>> - /*\n> >>> - * Import streams cannot be external.\n> >>> - *\n> >>> - * RAW capture is a special case where we simply copy the RAW\n> >>> - * buffer out of the request. All other buffer handling happens\n> >>> - * as if the stream is internal.\n> >>> - */\n> >>> - return external_ && !importOnly_;\n> >>> -}\n> >>> -\n> >>> -bool RPiStream::isImporter() const\n> >>> -{\n> >>> - return importOnly_;\n> >>> + return external_;\n> >>> }\n> >>>\n> >>> void RPiStream::reset()\n> >>> {\n> >>> external_ = false;\n> >>> - internalBuffers_.clear();\n> >>> + clearBuffers();\n> >>> }\n> >>>\n> >>> std::string RPiStream::name() const\n> >>> @@ -52,65 +42,123 @@ std::string RPiStream::name() const\n> >>> return name_;\n> >>> }\n> >>>\n> >>> -void RPiStream::setExternalBuffers(std::vector> *buffers)\n> >>> +void RPiStream::setExportedBuffers(std::vector> *buffers)\n> >>> {\n> >>> - externalBuffers_ = buffers;\n> >>> + std::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_),\n> >>> + [](std::unique_ptr &b) { return b.get(); });\n> >>> }\n> >>>\n> >>> -const std::vector> *RPiStream::getBuffers() const\n> >>> +const std::vector &RPiStream::getBuffers() const\n> >>> {\n> >>> - return external_ ? externalBuffers_ : &internalBuffers_;\n> >>> + return bufferList_;\n> >>> }\n> >>>\n> >>> void RPiStream::releaseBuffers()\n> >>> {\n> >>> dev_->releaseBuffers();\n> >>> - if (!external_ && !importOnly_)\n> >>> - internalBuffers_.clear();\n> >>> + clearBuffers();\n> >>> }\n> >>>\n> >>> -int RPiStream::importBuffers(unsigned int count)\n> >>> +int RPiStream::prepareBuffers(unsigned int count)\n> >>> {\n> >>> + int ret;\n> >>> +\n> >>> + if (!importOnly_) {\n> >>> + if (count) {\n> >>> + /* Export some frame buffers for internal use. */\n> >>> + ret = dev_->exportBuffers(count, &internalBuffers_);\n> >>> + if (ret < 0)\n> >>> + return ret;\n> >>> +\n> >>> + /* Add these exported buffers to the internal/external buffer list. */\n> >>> + setExportedBuffers(&internalBuffers_);\n> >>> +\n> >>> + /* Add these buffers to the queue of internal usable buffers. */\n> >>> + for (auto const &buffer : internalBuffers_)\n> >>> + availableBuffers_.push(buffer.get());\n> >>> + }\n> >>> +\n> >>> + /* We must import all internal/external exported buffers. */\n> >>> + count = bufferList_.size();\n> >>> + }\n> >>> +\n> >>> return dev_->importBuffers(count);\n> >>> }\n> >>>\n> >>> -int RPiStream::allocateBuffers(unsigned int count)\n> >>> +int RPiStream::queueAllBuffers()\n> >>> {\n> >>> - return dev_->allocateBuffers(count, &internalBuffers_);\n> >>> -}\n> >>> + int ret;\n> >>>\n> >>> -int RPiStream::queueBuffers()\n> >>> -{\n> >>> if (external_)\n> >>> return 0;\n> >>>\n> >>> - for (auto &b : internalBuffers_) {\n> >>> - int ret = dev_->queueBuffer(b.get());\n> >>> - if (ret) {\n> >>> - LOG(RPISTREAM, Error) << \"Failed to queue buffers for \"\n> >>> - << name_;\n> >>> + while (!availableBuffers_.empty()) {\n> >>> + ret = queueBuffer(availableBuffers_.front());\n> >>> + if (ret < 0)\n> >>> return ret;\n> >>> - }\n> >>> +\n> >>> + availableBuffers_.pop();\n> >>> }\n> >>>\n> >>> return 0;\n> >>> }\n> >>>\n> >>> -bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const\n> >>> +int RPiStream::queueBuffer(FrameBuffer *buffer)\n> >>> +{\n> >>> + /*\n> >>> + * A nullptr buffer implies an external stream, but no external\n> >>> + * buffer has been supplied. So, pick one from the availableBuffers_\n> >>> + * queue.\n> \n> Aha, now I see this here, maybe the comment addition I suggested above\n> is possibly redundant, as it's explained here ...\n> \n> >>> + */\n> >>> + if (!buffer) {\n> >>> + if (availableBuffers_.empty()) {\n> >>> + LOG(RPISTREAM, Warning) << \"No buffers available for \"\n> >>> + << name_;\n> >>> + return -EINVAL;\n> >>> + }\n> >>> +\n> >>> + buffer = availableBuffers_.front();\n> >>> + availableBuffers_.pop();\n> >>> + }\n> >>> +\n> >>> + LOG(RPISTREAM, Debug) << \"Queuing buffer \" << buffer->cookie()\n> >>> + << \" for \" << name_;\n> >>> +\n> >>> + int ret = dev_->queueBuffer(buffer);\n> >>> + if (ret) {\n> >>> + LOG(RPISTREAM, Error) << \"Failed to queue buffer for \"\n> >>> + << name_;\n> >>> + }\n> >>> +\n> >>> + return ret;\n> >>> +}\n> >>> +\n> >>> +void RPiStream::returnBuffer(FrameBuffer *buffer)\n> >>> {\n> >>> - auto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();\n> >>> - auto end = external_ ? externalBuffers_->end() : internalBuffers_.end();\n> >>> + /* This can only be called for external streams. */\n> >>> + assert(external_);\n> \n> Could this use ASSERT() from libcamera/internal/log.h ?\n> \n> >>> +\n> >>> + availableBuffers_.push(buffer);\n> >>> +}\n> >>>\n> >>> +bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const\n> >>> +{\n> >>> if (importOnly_)\n> >>> return false;\n> >>>\n> >>> - if (std::find_if(start, end,\n> >>> - [buffer](std::unique_ptr const &ref) { return ref.get() == buffer; }) != end)\n> >>> + if (std::find(bufferList_.begin(), bufferList_.end(), buffer) != bufferList_.end())\n> >>> return true;\n> >>>\n> >>> return false;\n> >>> }\n> >>>\n> >>> +void RPiStream::clearBuffers()\n> >>> +{\n> >>> + availableBuffers_ = std::queue{};\n> >>> + internalBuffers_.clear();\n> >>> + bufferList_.clear();\n> >>> +}\n> >>> +\n> >>> } /* namespace RPi */\n> >>>\n> >>> } /* namespace libcamera */\n> >>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> >>> index 3957e342..af9c2ad2 100644\n> >>> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> >>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> >>> @@ -39,21 +39,22 @@ public:\n> >>> V4L2VideoDevice *dev() const;\n> >>> void setExternal(bool external);\n> >>> bool isExternal() const;\n> >>> - bool isImporter() const;\n> >>> void reset();\n> >>> std::string name() const;\n> >>> - void setExternalBuffers(std::vector> *buffers);\n> >>> - const std::vector> *getBuffers() const;\n> >>> + void setExportedBuffers(std::vector> *buffers);\n> >>> + const std::vector &getBuffers() const;\n> >>> void releaseBuffers();\n> >>> - int importBuffers(unsigned int count);\n> >>> - int allocateBuffers(unsigned int count);\n> >>> - int queueBuffers();\n> >>> + int prepareBuffers(unsigned int count);\n> >>> + int queueAllBuffers();\n> >>> + int queueBuffer(FrameBuffer *buffer);\n> >>> + void returnBuffer(FrameBuffer *buffer);\n> >>> bool findFrameBuffer(FrameBuffer *buffer) const;\n> >>\n> >> Perhaps we should break those up into sections in patch 1/9. It's hard\n> >> to see the wood-for-the-trees in the wall of function declarations.\n> > \n> > Not sure I follow what you mean?\n> \n> There is a lot of text all bunched up together, so it's hard to get my\n> eyes around the class members.\n> \n> It would be nice to group functions with some line breaks to make it\n> easier on the eye.\n> \n> But that should happen in patch 1/9, so I'll go comment on it there. I\n> skipped it before because that patch just moves code around, but I thnik\n> it can also improve formatting for readability.\n> \n> >>>\n> >>> private:\n> >>> + void clearBuffers();\n> >>> /*\n> >>> * Indicates that this stream is active externally, i.e. the buffers\n> >>> - * are provided by the application.\n> >>> + * might be provided by (and returned to) the application.\n> >>> */\n> >>> bool external_;\n> >>> /* Indicates that this stream only imports buffers, e.g. ISP input. */\n> >>> @@ -62,10 +63,19 @@ private:\n> >>> std::string name_;\n> >>> /* The actual device stream. */\n> >>> std::unique_ptr dev_;\n> >>> - /* Internally allocated framebuffers associated with this device stream. */\n> >>> + /* All framebuffers associated with this device stream. */\n> >>> + std::vector bufferList_;\n> >>> + /*\n> >>> + * List of frame buffer that we can use if none have been provided by\n> >>> + * the application for external streams. This is populated by the\n> >>> + * buffers exported internally.\n> >>> + */\n> >>> + std::queue availableBuffers_;\n> >>> + /*\n> >>> + * This is a list of buffers exported internally. Need to keep this around\n> >>> + * as the stream needs to maintain ownership of these buffers.\n> >>> + */\n> >>> std::vector> internalBuffers_;\n> >>> - /* Externally allocated framebuffers associated with this device stream. */\n> >>> - std::vector> *externalBuffers_;\n> >>\n> >> Likewise here, some whitespace to break up these members would really\n> >> help, as by the time I've got here I've got definite review fatigue and\n> >> lots of lines close together just become a blur ... :S\n> > \n> > That's not a problem.\n> \n> But for this patch, I can't see anything wrong ... and I can see how\n> it's helping manage buffers to allow optional buffers on a stream for\n> RAW zero copy support.\n> \n> Reviewed-by: Kieran Bingham \n> \n> >>> };\n> >>>\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 83397C2E68\n\tfor ;\n\tWed, 22 Jul 2020 15:25:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 19A6960C34;\n\tWed, 22 Jul 2020 17:25:27 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E67CA60540\n\tfor ;\n\tWed, 22 Jul 2020 17:25:25 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CCBFB329;\n\tWed, 22 Jul 2020 17:25:18 +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=\"wbUT7IXh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1595431519;\n\tbh=kHQTmTXi4esrBPAdJLjqPhEUmuKbZrDI6c08LA6eYTE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wbUT7IXhuHjlODoyeeQrX3Y4INv73dH8asoXkw8CNi/X2Lf0ZoYJalY1N9/zdZs/Q\n\tlHWt3NtFFgoN34iEBnJx7yX9YQRmoZP6+B0KaY0UwFT6SOPqw+sZg1BshE1KUqyHos\n\terq4E/EmTF3jyzc6CHR7mtoTjmP9Y+A7MGZGCRz4=","Date":"Wed, 22 Jul 2020 18:25:13 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"<20200722152513.GH29813@pendragon.ideasonboard.com>","References":"<20200720091311.805092-1-naush@raspberrypi.com>\n\t<20200720091311.805092-7-naush@raspberrypi.com>\n\t<4577acb0-3976-4721-6673-e456db4dc4b3@ideasonboard.com>\n\t\n\t<2a259e2a-9cb6-73c3-26f6-328a78077b48@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<2a259e2a-9cb6-73c3-26f6-328a78077b48@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 6/9] libcamera: pipeline:\n\traspberrypi: Rework stream buffer logic for zero-copy","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11508,"web_url":"https://patchwork.libcamera.org/comment/11508/","msgid":"","date":"2020-07-22T15:33:08","subject":"Re: [libcamera-devel] [PATCH v4 6/9] libcamera: pipeline:\n\traspberrypi: Rework stream buffer logic for zero-copy","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for the review.\n\nOn Wed, 22 Jul 2020 at 16:25, Laurent Pinchart\n wrote:\n>\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Wed, Jul 22, 2020 at 01:21:33PM +0100, Kieran Bingham wrote:\n> > On 22/07/2020 09:04, Naushir Patuck wrote:\n> > > On Tue, 21 Jul 2020 at 16:41, Kieran Bingham wrote:\n> > >> On 20/07/2020 10:13, Naushir Patuck wrote:\n> > >>> Stop using v4l2_videodevice::allocateBuffer() for internal buffers and\n> > >>> instead export/import all buffers. This allows the pipeline to return\n> > >>> any stream buffer requested by the application as zero-copy.\n> > >>>\n> > >>> Advertise the Unicam Image stream as the RAW capture stream now.\n> > >>>\n> > >>> The RPiStream object now maintains a new list of buffers that are\n> > >>> available to queue into a device. This is needed to distinguish between\n> > >>> FrameBuffers allocated for internal use vs externally provided buffers.\n> > >>> When a Request comes in, if a buffer is not provided for an exported\n> > >>> stream, we re-use a buffer from this list.\n> > >>>\n> > >>> Signed-off-by: Naushir Patuck \n> > >>> Reviewed-by: Niklas Söderlund \n> > >>\n> > >> A few small nit fixups below but they could possibly be fixed while\n> > >> applying anyway if you prefer.\n> > >>\n> > >> I've hit review fatigue though, so I'll look at this again tomorrow.\n> > >\n> > > I understand :)\n> > > I will make the necessary fixups based on your review comments, and\n> > > submit a new patch set. Will wait until you have finished reviewing\n> > > this one before I send the next revision.\n> >\n> > Looking good, I think the only actionable thing remaining here is\n> > potentially to use ASSERT() over assert() :-)\n> >\n> > >>> ---\n> > >>> .../pipeline/raspberrypi/raspberrypi.cpp | 228 ++++++++++--------\n> > >>> .../pipeline/raspberrypi/rpi_stream.cpp | 122 +++++++---\n> > >>> .../pipeline/raspberrypi/rpi_stream.h | 30 ++-\n> > >>> 3 files changed, 226 insertions(+), 154 deletions(-)\n> > >>>\n> > >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >>> index 8f6a999b..dbc22521 100644\n> > >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >>> @@ -187,7 +187,8 @@ private:\n> > >>> void checkRequestCompleted();\n> > >>> void tryRunPipeline();\n> > >>> void tryFlushQueues();\n> > >>> - FrameBuffer *updateQueue(std::queue &q, uint64_t timestamp, V4L2VideoDevice *dev);\n> > >>> + FrameBuffer *updateQueue(std::queue &q, uint64_t timestamp,\n> > >>> + RPi::RPiStream *stream);\n> > >>>\n> > >>> unsigned int ispOutputCount_;\n> > >>> };\n> > >>> @@ -508,8 +509,15 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > >>> StreamConfiguration &cfg = config->at(i);\n> > >>>\n> > >>> if (isRaw(cfg.pixelFormat)) {\n> > >>> - cfg.setStream(&data->isp_[Isp::Input]);\n> > >>> - data->isp_[Isp::Input].setExternal(true);\n> > >>> + cfg.setStream(&data->unicam_[Unicam::Image]);\n> > >>> + /*\n> > >>> + * We must set both Unicam streams as external, even\n> > >>> + * though the application may only request RAW frames.\n> > >>> + * This is because we match timestamps on both streams\n> > >>> + * to synchronise buffers.\n> > >>> + */\n> > >>> + data->unicam_[Unicam::Image].setExternal(true);\n> > >>> + data->unicam_[Unicam::Embedded].setExternal(true);\n> > >>> continue;\n> > >>> }\n> > >>>\n> > >>> @@ -612,7 +620,7 @@ int PipelineHandlerRPi::exportFrameBuffers(Camera *camera, Stream *stream,\n> > >>> unsigned int count = stream->configuration().bufferCount;\n> > >>> int ret = s->dev()->exportBuffers(count, buffers);\n> > >>>\n> > >>> - s->setExternalBuffers(buffers);\n> > >>> + s->setExportedBuffers(buffers);\n> > >>>\n> > >>> return ret;\n> > >>> }\n> > >>> @@ -712,14 +720,23 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> > >>> if (data->state_ == RPiCameraData::State::Stopped)\n> > >>> return -EINVAL;\n> > >>>\n> > >>> - /* Ensure all external streams have associated buffers! */\n> > >>> - for (auto &stream : data->isp_) {\n> > >>> - if (!stream.isExternal())\n> > >>> - continue;\n> > >>> + LOG(RPI, Debug) << \"queueRequestDevice: New request.\";\n> > >>>\n> > >>> - if (!request->findBuffer(&stream)) {\n> > >>> - LOG(RPI, Error) << \"Attempt to queue request with invalid stream.\";\n> > >>> - return -ENOENT;\n> > >>> + /* Push all buffers supplied in the Request to the respective streams. */\n> > >>> + for (auto stream : data->streams_) {\n> > >>> + if (stream->isExternal()) {\n> > >>\n> > >> Are all streams now 'external' ?\n> > >\n> > > All streams but the ISP input (which only imports buffers) *can* be\n> > > external. They are marked external only if the app configures the\n> > > pipeline handler by saying it may request a buffer from the stream.\n> > > In reality, all streams could be marked external all the time, but the\n> > > buffer handling will take a slightly less efficient path, so I kept\n> > > the distinction.\n> > >\n> > >>> + FrameBuffer *buffer = request->findBuffer(stream);\n> > >>> + /*\n> > >>> + * A nullptr in buffer means that we should queue an internally\n> > >>> + * allocated buffer.\n> >\n> > Now I understand what's going on here, perhaps this could be expanded a\n> > bit to explain?\n> >\n> > \"If no buffer is provided by the request for this stream, we queue a\n> > nullptr to the stream to signify that it must use an internally\n> > allocated buffer for this capture request which will not be given back\n> > to the application, but can be used to support the internal pipeline flow.\"\n> >\n> > Now I see how this leads towards zero-copy raw support ;-)\n> >\n> > >>> + *\n> > >>> + * The below queueBuffer() call will do nothing if there are not\n> > >>> + * enough internal buffers allocated, but this will be handled by\n> > >>> + * queuing the request for buffers in the RPiStream object.\n> > >>> + */\n> > >>> + int ret = stream->queueBuffer(buffer);\n> > >>> + if (ret)\n> > >>> + return ret;\n> > >>> }\n> > >>> }\n> > >>>\n> > >>> @@ -808,12 +825,10 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > >>> /* Initialize the camera properties. */\n> > >>> data->properties_ = data->sensor_->properties();\n> > >>>\n> > >>> - /*\n> > >>> - * List the available output streams.\n> > >>> - * Currently cannot do Unicam streams!\n> > >>> - */\n> > >>> + /*List the available streams an application may request. */\n> > >>\n> > >> Minor nit, there's a space missing between /* and List ...\n> > >>\n> > >>> std::set streams;\n> > >>> - streams.insert(&data->isp_[Isp::Input]);\n> > >>> + streams.insert(&data->unicam_[Unicam::Image]);\n> > >>> + streams.insert(&data->unicam_[Unicam::Embedded]);\n>\n> Can an application request the embedded data ?\n\nWith this change, we can now advertise embedded data as well as isp\nstatistics available for the application to request. The only thing\nmissing is a way for the application to do so. In a separate\ndiscussion we talked about libcamera::PixelFormat not being the right\nplace as these are data formats, not image formats. No conclusion has\nbeen reached yet, but I will resurrect that discussion shortly :-)\n\n>\n> > >>> streams.insert(&data->isp_[Isp::Output0]);\n> > >>> streams.insert(&data->isp_[Isp::Output1]);\n> > >>> streams.insert(&data->isp_[Isp::Stats]);\n> > >>> @@ -831,9 +846,28 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> > >>> int ret;\n> > >>>\n> > >>> for (auto const stream : data->streams_) {\n> > >>> - ret = stream->queueBuffers();\n> > >>> - if (ret < 0)\n> > >>> - return ret;\n> > >>> + if (!stream->isExternal()) {\n> > >>> + ret = stream->queueAllBuffers();\n> > >>> + if (ret < 0)\n> > >>> + return ret;\n> > >>> + } else {\n> > >>> + /*\n> > >>> + * For external streams, we must queue up a set of internal\n> > >>> + * buffers to handle the number of drop frames requested by\n> > >>> + * the IPA. This is done by passing nullptr in queueBuffer().\n> > >>> + *\n> > >>> + * The below queueBuffer() call will do nothing if there\n> > >>> + * are not enough internal buffers allocated, but this will\n> > >>> + * be handled by queuing the request for buffers in the\n> > >>> + * RPiStream object.\n> > >>> + */\n> > >>> + unsigned int i;\n> > >>> + for (i = 0; i < data->dropFrameCount_; i++) {\n> > >>> + int ret = stream->queueBuffer(nullptr);\n> > >>> + if (ret)\n> > >>> + return ret;\n> > >>> + }\n> > >>> + }\n> > >>> }\n> > >>>\n> > >>> return 0;\n> > >>> @@ -847,7 +881,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > >>> /*\n> > >>> * Decide how many internal buffers to allocate. For now, simply look\n> > >>> * at how many external buffers will be provided. Will need to improve\n> > >>> - * this logic.\n> > >>> + * this logic. However, we really must have all stream allocate the same\n> > >>\n> > >> s/stream/streams/\n> > >>\n> > >>> + * number of buffers to simplify error handling in queueRequestDevice().\n> > >>> */\n> > >>\n> > >> Does this include Raw streams? I thought that allocates less buffers? Or\n> > >> perhaps it's the same number of internal buffers, and only 2 extra\n> > >> external buffers for that case...\n> > >\n> > > That's correct. For now all (including Raw) streams allocate the same\n> > > number of internal buffers. Hence the comment that we could improve\n> > > this logic, it is a bit wasteful on memory. This bit does need\n> > > refinement at some point.\n> > >\n> > >>> unsigned int maxBuffers = 0;\n> > >>> for (const Stream *s : camera->streams())\n> > >>> @@ -855,33 +890,9 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > >>> maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);\n> > >>>\n> > >>> for (auto const stream : data->streams_) {\n> > >>> - if (stream->isExternal() || stream->isImporter()) {\n> > >>> - /*\n> > >>> - * If a stream is marked as external reserve memory to\n> > >>> - * prepare to import as many buffers are requested in\n> > >>> - * the stream configuration.\n> > >>> - *\n> > >>> - * If a stream is an internal stream with importer\n> > >>> - * role, reserve as many buffers as possible.\n> > >>> - */\n> > >>> - unsigned int count = stream->isExternal()\n> > >>> - ? stream->configuration().bufferCount\n> > >>> - : maxBuffers;\n> > >>> - ret = stream->importBuffers(count);\n> > >>> - if (ret < 0)\n> > >>> - return ret;\n> > >>> - } else {\n> > >>> - /*\n> > >>> - * If the stream is an internal exporter allocate and\n> > >>> - * export as many buffers as possible to its internal\n> > >>> - * pool.\n> > >>> - */\n> > >>> - ret = stream->allocateBuffers(maxBuffers);\n> > >>> - if (ret < 0) {\n> > >>> - freeBuffers(camera);\n> > >>> - return ret;\n> > >>> - }\n> > >>> - }\n> > >>> + ret = stream->prepareBuffers(maxBuffers);\n> > >>> + if (ret < 0)\n> > >>> + return ret;\n>\n> This doesn't free buffers anymore on failure, is that an oversight ?\n\nOversight. Will fix.\n\n>\n> > >>> }\n> > >>>\n> > >>> /*\n> > >>> @@ -889,7 +900,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > >>> * the IPA and RPI_IPA_EVENT_SIGNAL_ISP_PREPARE event.\n> > >>> */\n> > >>> count = 0;\n> > >>> - for (auto const &b : *data->unicam_[Unicam::Image].getBuffers()) {\n> > >>> + for (auto const &b : data->unicam_[Unicam::Image].getBuffers()) {\n> > >>> b->setCookie(count++);\n> > >>> }\n> > >>>\n> > >>> @@ -898,14 +909,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > >>> * the IPA.\n> > >>> */\n> > >>> count = 0;\n> > >>> - for (auto const &b : *data->isp_[Isp::Stats].getBuffers()) {\n> > >>> + for (auto const &b : data->isp_[Isp::Stats].getBuffers()) {\n> > >>> b->setCookie(count++);\n> > >>> data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b->cookie(),\n> > >>> .planes = b->planes() });\n> > >>> }\n> > >>>\n> > >>> count = 0;\n> > >>> - for (auto const &b : *data->unicam_[Unicam::Embedded].getBuffers()) {\n> > >>> + for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {\n> > >>> b->setCookie(count++);\n> > >>> data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b->cookie(),\n> > >>> .planes = b->planes() });\n> > >>> @@ -1066,7 +1077,7 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n> > >>> switch (action.operation) {\n> > >>> case RPI_IPA_ACTION_STATS_METADATA_COMPLETE: {\n> > >>> unsigned int bufferId = action.data[0];\n> > >>> - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers()->at(bufferId).get();\n> > >>> + FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> > >>>\n> > >>> handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> > >>> /* Fill the Request metadata buffer with what the IPA has provided */\n> > >>> @@ -1077,19 +1088,19 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n> > >>>\n> > >>> case RPI_IPA_ACTION_EMBEDDED_COMPLETE: {\n> > >>> unsigned int bufferId = action.data[0];\n> > >>> - FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers()->at(bufferId).get();\n> > >>> + FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);\n> > >>> handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> > >>> break;\n> > >>> }\n> > >>>\n> > >>> case RPI_IPA_ACTION_RUN_ISP: {\n> > >>> unsigned int bufferId = action.data[0];\n> > >>> - FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers()->at(bufferId).get();\n> > >>> + FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);\n> > >>>\n> > >>> LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << buffer->cookie()\n> > >>> << \", timestamp: \" << buffer->metadata().timestamp;\n> > >>>\n> > >>> - isp_[Isp::Input].dev()->queueBuffer(buffer);\n> > >>> + isp_[Isp::Input].queueBuffer(buffer);\n> > >>> ispOutputCount_ = 0;\n> > >>> break;\n> > >>> }\n> > >>> @@ -1190,22 +1201,27 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n> > >>> << \", buffer id \" << buffer->cookie()\n> > >>> << \", timestamp: \" << buffer->metadata().timestamp;\n> > >>>\n> > >>> - handleStreamBuffer(buffer, stream);\n> > >>> -\n> > >>> /*\n> > >>> - * Increment the number of ISP outputs generated.\n> > >>> - * This is needed to track dropped frames.\n> > >>> + * ISP statistics buffer must not be re-queued or sent back to the\n> > >>> + * application until after the IPA signals so.\n> > >>> */\n> > >>> - ispOutputCount_++;\n> > >>> -\n> > >>> - /* If this is a stats output, hand it to the IPA now. */\n> > >>> if (stream == &isp_[Isp::Stats]) {\n> > >>> IPAOperationData op;\n> > >>> op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;\n> > >>> op.data = { RPiIpaMask::STATS | buffer->cookie() };\n> > >>> ipa_->processEvent(op);\n> > >>> + } else {\n> > >>> + /* Any other ISP output can be handed back to the application now. */\n> > >>> + handleStreamBuffer(buffer, stream);\n> > >>> }\n> > >>>\n> > >>> + /*\n> > >>> + * Increment the number of ISP outputs generated.\n> > >>> + * This is needed to track dropped frames.\n> > >>> + */\n> > >>> + ispOutputCount_++;\n> > >>> +\n> > >>> +\n>\n> Extra blank line ?\n>\n> > >>> handleState();\n> > >>> }\n> > >>>\n> > >>> @@ -1218,8 +1234,12 @@ void RPiCameraData::clearIncompleteRequests()\n> > >>> */\n> > >>> for (auto const request : requestQueue_) {\n> > >>> for (auto const stream : streams_) {\n> > >>> - if (stream->isExternal())\n> > >>> - stream->dev()->queueBuffer(request->findBuffer(stream));\n> > >>> + if (!stream->isExternal())\n> > >>> + continue;\n> > >>> +\n> > >>> + FrameBuffer *buffer = request->findBuffer(stream);\n> > >>> + if (buffer)\n> > >>> + stream->queueBuffer(buffer);\n> > >>> }\n> > >>> }\n> > >>>\n> > >>> @@ -1248,7 +1268,7 @@ void RPiCameraData::clearIncompleteRequests()\n> > >>> * Has the buffer already been handed back to the\n> > >>> * request? If not, do so now.\n> > >>> */\n> > >>> - if (buffer->request())\n> > >>> + if (buffer && buffer->request())\n> > >>> pipe_->completeBuffer(camera_, request, buffer);\n> > >>> }\n> > >>>\n> > >>> @@ -1260,30 +1280,24 @@ void RPiCameraData::clearIncompleteRequests()\n> > >>> void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)\n> > >>> {\n> > >>> if (stream->isExternal()) {\n> > >>> - if (!dropFrameCount_) {\n> > >>> - Request *request = buffer->request();\n> > >>> + Request *request = requestQueue_.front();\n> > >>> +\n> > >>> + if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {\n> > >>> + /*\n> > >>> + * Tag the buffer as completed, returning it to the\n> > >>> + * application.\n> > >>> + */\n> > >>> pipe_->completeBuffer(camera_, request, buffer);\n> > >>> + } else {\n> > >>> + /*\n> > >>> + * This buffer was not part of the Request, so we can\n> > >>> + * recycle it.\n> > >>> + */\n> > >>> + stream->returnBuffer(buffer);\n> > >>> }\n> > >>> } else {\n> > >>> - /* Special handling for RAW buffer Requests.\n> > >>> - *\n> > >>> - * The ISP input stream is alway an import stream, but if the\n> > >>> - * current Request has been made for a buffer on the stream,\n> > >>> - * simply memcpy to the Request buffer and requeue back to the\n> > >>> - * device.\n> > >>> - */\n> > >>> - if (stream == &unicam_[Unicam::Image] && !dropFrameCount_) {\n> > >>> - const Stream *rawStream = static_cast(&isp_[Isp::Input]);\n> > >>> - Request *request = requestQueue_.front();\n> > >>> - FrameBuffer *raw = request->findBuffer(const_cast(rawStream));\n> > >>> - if (raw) {\n> > >>> - raw->copyFrom(buffer);\n> > >>> - pipe_->completeBuffer(camera_, request, raw);\n> > >>> - }\n> > >>> - }\n> > >>> -\n> > >>> - /* Simply requeue the buffer. */\n> > >>> - stream->dev()->queueBuffer(buffer);\n> > >>> + /* Simply re-queue the buffer to the requested stream. */\n> > >>> + stream->queueBuffer(buffer);\n> > >>> }\n> > >>> }\n> > >>>\n> > >>> @@ -1367,7 +1381,7 @@ void RPiCameraData::tryRunPipeline()\n> > >>> * current bayer buffer will be removed and re-queued to the driver.\n> > >>> */\n> > >>> embeddedBuffer = updateQueue(embeddedQueue_, bayerBuffer->metadata().timestamp,\n> > >>> - unicam_[Unicam::Embedded].dev());\n> > >>> + &unicam_[Unicam::Embedded]);\n> > >>>\n> > >>> if (!embeddedBuffer) {\n> > >>> LOG(RPI, Debug) << \"Could not find matching embedded buffer\";\n> > >>> @@ -1386,7 +1400,7 @@ void RPiCameraData::tryRunPipeline()\n> > >>>\n> > >>> embeddedBuffer = embeddedQueue_.front();\n> > >>> bayerBuffer = updateQueue(bayerQueue_, embeddedBuffer->metadata().timestamp,\n> > >>> - unicam_[Unicam::Image].dev());\n> > >>> + &unicam_[Unicam::Image]);\n> > >>>\n> > >>> if (!bayerBuffer) {\n> > >>> LOG(RPI, Debug) << \"Could not find matching bayer buffer - ending.\";\n> > >>> @@ -1394,11 +1408,7 @@ void RPiCameraData::tryRunPipeline()\n> > >>> }\n> > >>> }\n> > >>>\n> > >>> - /*\n> > >>> - * Take the first request from the queue and action the IPA.\n> > >>> - * Unicam buffers for the request have already been queued as they come\n> > >>> - * in.\n> > >>> - */\n> > >>> + /* Take the first request from the queue and action the IPA. */\n> > >>> Request *request = requestQueue_.front();\n> > >>>\n> > >>> /*\n> > >>> @@ -1410,12 +1420,6 @@ void RPiCameraData::tryRunPipeline()\n> > >>> op.controls = { request->controls() };\n> > >>> ipa_->processEvent(op);\n> > >>>\n> > >>> - /* Queue up any ISP buffers passed into the request. */\n> > >>> - for (auto &stream : isp_) {\n> > >>> - if (stream.isExternal())\n> > >>> - stream.dev()->queueBuffer(request->findBuffer(&stream));\n> > >>> - }\n> > >>> -\n> > >>> /* Ready to use the buffers, pop them off the queue. */\n> > >>> bayerQueue_.pop();\n> > >>> embeddedQueue_.pop();\n> > >>> @@ -1445,32 +1449,42 @@ void RPiCameraData::tryFlushQueues()\n> > >>> * and give a chance for the hardware to return to lock-step. We do have\n> > >>> * to drop all interim frames.\n> > >>> */\n> > >>> - if (unicam_[Unicam::Image].getBuffers()->size() == bayerQueue_.size() &&\n> > >>> - unicam_[Unicam::Embedded].getBuffers()->size() == embeddedQueue_.size()) {\n> > >>> + if (unicam_[Unicam::Image].getBuffers().size() == bayerQueue_.size() &&\n> > >>> + unicam_[Unicam::Embedded].getBuffers().size() == embeddedQueue_.size()) {\n> > >>> + /* This cannot happen when Unicam streams are external. */\n> > >>> + assert(!unicam_[Unicam::Image].isExternal());\n> > >>> +\n> > >>> LOG(RPI, Warning) << \"Flushing all buffer queues!\";\n> > >>>\n> > >>> while (!bayerQueue_.empty()) {\n> > >>> - unicam_[Unicam::Image].dev()->queueBuffer(bayerQueue_.front());\n> > >>> + unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());\n> > >>> bayerQueue_.pop();\n> > >>> }\n> > >>>\n> > >>> while (!embeddedQueue_.empty()) {\n> > >>> - unicam_[Unicam::Embedded].dev()->queueBuffer(embeddedQueue_.front());\n> > >>> + unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());\n> > >>> embeddedQueue_.pop();\n> > >>> }\n> > >>> }\n> > >>> }\n> > >>>\n> > >>> FrameBuffer *RPiCameraData::updateQueue(std::queue &q, uint64_t timestamp,\n> > >>> - V4L2VideoDevice *dev)\n> > >>> + RPi::RPiStream *stream)\n> > >>> {\n> > >>> + /*\n> > >>> + * If the unicam streams are external (both have to the same), then we\n> > >>\n> > >> both have to 'be' the same?\n> > >>\n> > >>> + * can only return out the top buffer in the queue, and assume they have\n> > >>> + * been synced by queuing at the same time. We cannot drop these frames,\n> > >>> + * as they may have been provided externally.\n> > >>> + */\n> > >>> while (!q.empty()) {\n> > >>> FrameBuffer *b = q.front();\n> > >>> - if (b->metadata().timestamp < timestamp) {\n> > >>> + if (!stream->isExternal() && b->metadata().timestamp < timestamp) {\n> > >>> q.pop();\n> > >>> - dev->queueBuffer(b);\n> > >>> - LOG(RPI, Warning) << \"Dropping input frame!\";\n> > >>> - } else if (b->metadata().timestamp == timestamp) {\n> > >>> + stream->queueBuffer(b);\n> > >>> + LOG(RPI, Warning) << \"Dropping unmatched input frame in stream \"\n> > >>> + << stream->name();\n> > >>> + } else if (stream->isExternal() || b->metadata().timestamp == timestamp) {\n> > >>> /* The calling function will pop the item from the queue. */\n> > >>> return b;\n> > >>> } else {\n> > >>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > >>> index 2edb8b59..02f8d3e0 100644\n> > >>> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > >>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > >>> @@ -21,30 +21,20 @@ V4L2VideoDevice *RPiStream::dev() const\n> > >>>\n> > >>> void RPiStream::setExternal(bool external)\n> > >>> {\n> > >>> + /* Import streams cannot be external. */\n> > >>> + assert(!external || !importOnly_);\n> > >>> external_ = external;\n> > >>> }\n> > >>>\n> > >>> bool RPiStream::isExternal() const\n> > >>> {\n> > >>> - /*\n> > >>> - * Import streams cannot be external.\n> > >>> - *\n> > >>> - * RAW capture is a special case where we simply copy the RAW\n> > >>> - * buffer out of the request. All other buffer handling happens\n> > >>> - * as if the stream is internal.\n> > >>> - */\n> > >>> - return external_ && !importOnly_;\n> > >>> -}\n> > >>> -\n> > >>> -bool RPiStream::isImporter() const\n> > >>> -{\n> > >>> - return importOnly_;\n> > >>> + return external_;\n> > >>> }\n> > >>>\n> > >>> void RPiStream::reset()\n> > >>> {\n> > >>> external_ = false;\n> > >>> - internalBuffers_.clear();\n> > >>> + clearBuffers();\n> > >>> }\n> > >>>\n> > >>> std::string RPiStream::name() const\n> > >>> @@ -52,65 +42,123 @@ std::string RPiStream::name() const\n> > >>> return name_;\n> > >>> }\n> > >>>\n> > >>> -void RPiStream::setExternalBuffers(std::vector> *buffers)\n> > >>> +void RPiStream::setExportedBuffers(std::vector> *buffers)\n> > >>> {\n> > >>> - externalBuffers_ = buffers;\n> > >>> + std::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_),\n> > >>> + [](std::unique_ptr &b) { return b.get(); });\n> > >>> }\n> > >>>\n> > >>> -const std::vector> *RPiStream::getBuffers() const\n> > >>> +const std::vector &RPiStream::getBuffers() const\n> > >>> {\n> > >>> - return external_ ? externalBuffers_ : &internalBuffers_;\n> > >>> + return bufferList_;\n> > >>> }\n> > >>>\n> > >>> void RPiStream::releaseBuffers()\n> > >>> {\n> > >>> dev_->releaseBuffers();\n> > >>> - if (!external_ && !importOnly_)\n> > >>> - internalBuffers_.clear();\n> > >>> + clearBuffers();\n> > >>> }\n> > >>>\n> > >>> -int RPiStream::importBuffers(unsigned int count)\n> > >>> +int RPiStream::prepareBuffers(unsigned int count)\n> > >>> {\n> > >>> + int ret;\n> > >>> +\n> > >>> + if (!importOnly_) {\n> > >>> + if (count) {\n> > >>> + /* Export some frame buffers for internal use. */\n> > >>> + ret = dev_->exportBuffers(count, &internalBuffers_);\n> > >>> + if (ret < 0)\n> > >>> + return ret;\n> > >>> +\n> > >>> + /* Add these exported buffers to the internal/external buffer list. */\n> > >>> + setExportedBuffers(&internalBuffers_);\n> > >>> +\n> > >>> + /* Add these buffers to the queue of internal usable buffers. */\n> > >>> + for (auto const &buffer : internalBuffers_)\n> > >>> + availableBuffers_.push(buffer.get());\n> > >>> + }\n> > >>> +\n> > >>> + /* We must import all internal/external exported buffers. */\n> > >>> + count = bufferList_.size();\n> > >>> + }\n> > >>> +\n> > >>> return dev_->importBuffers(count);\n> > >>> }\n> > >>>\n> > >>> -int RPiStream::allocateBuffers(unsigned int count)\n> > >>> +int RPiStream::queueAllBuffers()\n> > >>> {\n> > >>> - return dev_->allocateBuffers(count, &internalBuffers_);\n> > >>> -}\n> > >>> + int ret;\n> > >>>\n> > >>> -int RPiStream::queueBuffers()\n> > >>> -{\n> > >>> if (external_)\n> > >>> return 0;\n> > >>>\n> > >>> - for (auto &b : internalBuffers_) {\n> > >>> - int ret = dev_->queueBuffer(b.get());\n> > >>> - if (ret) {\n> > >>> - LOG(RPISTREAM, Error) << \"Failed to queue buffers for \"\n> > >>> - << name_;\n> > >>> + while (!availableBuffers_.empty()) {\n> > >>> + ret = queueBuffer(availableBuffers_.front());\n> > >>> + if (ret < 0)\n> > >>> return ret;\n> > >>> - }\n> > >>> +\n> > >>> + availableBuffers_.pop();\n> > >>> }\n> > >>>\n> > >>> return 0;\n> > >>> }\n> > >>>\n> > >>> -bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const\n> > >>> +int RPiStream::queueBuffer(FrameBuffer *buffer)\n> > >>> +{\n> > >>> + /*\n> > >>> + * A nullptr buffer implies an external stream, but no external\n> > >>> + * buffer has been supplied. So, pick one from the availableBuffers_\n> > >>> + * queue.\n> >\n> > Aha, now I see this here, maybe the comment addition I suggested above\n> > is possibly redundant, as it's explained here ...\n> >\n> > >>> + */\n> > >>> + if (!buffer) {\n> > >>> + if (availableBuffers_.empty()) {\n> > >>> + LOG(RPISTREAM, Warning) << \"No buffers available for \"\n> > >>> + << name_;\n> > >>> + return -EINVAL;\n> > >>> + }\n> > >>> +\n> > >>> + buffer = availableBuffers_.front();\n> > >>> + availableBuffers_.pop();\n> > >>> + }\n> > >>> +\n> > >>> + LOG(RPISTREAM, Debug) << \"Queuing buffer \" << buffer->cookie()\n> > >>> + << \" for \" << name_;\n> > >>> +\n> > >>> + int ret = dev_->queueBuffer(buffer);\n> > >>> + if (ret) {\n> > >>> + LOG(RPISTREAM, Error) << \"Failed to queue buffer for \"\n> > >>> + << name_;\n> > >>> + }\n> > >>> +\n> > >>> + return ret;\n> > >>> +}\n> > >>> +\n> > >>> +void RPiStream::returnBuffer(FrameBuffer *buffer)\n> > >>> {\n> > >>> - auto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();\n> > >>> - auto end = external_ ? externalBuffers_->end() : internalBuffers_.end();\n> > >>> + /* This can only be called for external streams. */\n> > >>> + assert(external_);\n> >\n> > Could this use ASSERT() from libcamera/internal/log.h ?\n> >\n> > >>> +\n> > >>> + availableBuffers_.push(buffer);\n> > >>> +}\n> > >>>\n> > >>> +bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const\n> > >>> +{\n> > >>> if (importOnly_)\n> > >>> return false;\n> > >>>\n> > >>> - if (std::find_if(start, end,\n> > >>> - [buffer](std::unique_ptr const &ref) { return ref.get() == buffer; }) != end)\n> > >>> + if (std::find(bufferList_.begin(), bufferList_.end(), buffer) != bufferList_.end())\n> > >>> return true;\n> > >>>\n> > >>> return false;\n> > >>> }\n> > >>>\n> > >>> +void RPiStream::clearBuffers()\n> > >>> +{\n> > >>> + availableBuffers_ = std::queue{};\n> > >>> + internalBuffers_.clear();\n> > >>> + bufferList_.clear();\n> > >>> +}\n> > >>> +\n> > >>> } /* namespace RPi */\n> > >>>\n> > >>> } /* namespace libcamera */\n> > >>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > >>> index 3957e342..af9c2ad2 100644\n> > >>> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > >>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > >>> @@ -39,21 +39,22 @@ public:\n> > >>> V4L2VideoDevice *dev() const;\n> > >>> void setExternal(bool external);\n> > >>> bool isExternal() const;\n> > >>> - bool isImporter() const;\n> > >>> void reset();\n> > >>> std::string name() const;\n> > >>> - void setExternalBuffers(std::vector> *buffers);\n> > >>> - const std::vector> *getBuffers() const;\n> > >>> + void setExportedBuffers(std::vector> *buffers);\n> > >>> + const std::vector &getBuffers() const;\n> > >>> void releaseBuffers();\n> > >>> - int importBuffers(unsigned int count);\n> > >>> - int allocateBuffers(unsigned int count);\n> > >>> - int queueBuffers();\n> > >>> + int prepareBuffers(unsigned int count);\n> > >>> + int queueAllBuffers();\n> > >>> + int queueBuffer(FrameBuffer *buffer);\n> > >>> + void returnBuffer(FrameBuffer *buffer);\n> > >>> bool findFrameBuffer(FrameBuffer *buffer) const;\n> > >>\n> > >> Perhaps we should break those up into sections in patch 1/9. It's hard\n> > >> to see the wood-for-the-trees in the wall of function declarations.\n> > >\n> > > Not sure I follow what you mean?\n> >\n> > There is a lot of text all bunched up together, so it's hard to get my\n> > eyes around the class members.\n> >\n> > It would be nice to group functions with some line breaks to make it\n> > easier on the eye.\n> >\n> > But that should happen in patch 1/9, so I'll go comment on it there. I\n> > skipped it before because that patch just moves code around, but I thnik\n> > it can also improve formatting for readability.\n> >\n> > >>>\n> > >>> private:\n> > >>> + void clearBuffers();\n> > >>> /*\n> > >>> * Indicates that this stream is active externally, i.e. the buffers\n> > >>> - * are provided by the application.\n> > >>> + * might be provided by (and returned to) the application.\n> > >>> */\n> > >>> bool external_;\n> > >>> /* Indicates that this stream only imports buffers, e.g. ISP input. */\n> > >>> @@ -62,10 +63,19 @@ private:\n> > >>> std::string name_;\n> > >>> /* The actual device stream. */\n> > >>> std::unique_ptr dev_;\n> > >>> - /* Internally allocated framebuffers associated with this device stream. */\n> > >>> + /* All framebuffers associated with this device stream. */\n> > >>> + std::vector bufferList_;\n> > >>> + /*\n> > >>> + * List of frame buffer that we can use if none have been provided by\n> > >>> + * the application for external streams. This is populated by the\n> > >>> + * buffers exported internally.\n> > >>> + */\n> > >>> + std::queue availableBuffers_;\n> > >>> + /*\n> > >>> + * This is a list of buffers exported internally. Need to keep this around\n> > >>> + * as the stream needs to maintain ownership of these buffers.\n> > >>> + */\n> > >>> std::vector> internalBuffers_;\n> > >>> - /* Externally allocated framebuffers associated with this device stream. */\n> > >>> - std::vector> *externalBuffers_;\n> > >>\n> > >> Likewise here, some whitespace to break up these members would really\n> > >> help, as by the time I've got here I've got definite review fatigue and\n> > >> lots of lines close together just become a blur ... :S\n> > >\n> > > That's not a problem.\n> >\n> > But for this patch, I can't see anything wrong ... and I can see how\n> > it's helping manage buffers to allow optional buffers on a stream for\n> > RAW zero copy support.\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> > >>> };\n> > >>>\n> > >>> /*\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n\nRegards,\nNaush","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 1B01FBDB1B\n\tfor ;\n\tWed, 22 Jul 2020 15:33:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DABF160C34;\n\tWed, 22 Jul 2020 17:33:27 +0200 (CEST)","from mail-lf1-x129.google.com (mail-lf1-x129.google.com\n\t[IPv6:2a00:1450:4864:20::129])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1035760540\n\tfor ;\n\tWed, 22 Jul 2020 17:33:27 +0200 (CEST)","by mail-lf1-x129.google.com with SMTP id i80so1540960lfi.13\n\tfor ;\n\tWed, 22 Jul 2020 08:33:27 -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=\"PBEOlCD9\"; 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=FKWssiyrPfvNtQaelPVj9kFM55Cf4kWT8vePQJSEROc=;\n\tb=PBEOlCD9kYQYNLzM5H3pxCFxyrHvGd90PW+0fcu4wOn9x2ABf/LbqMAooE1kd8/KAD\n\tlLcuOnYUkaMiWsW1a/bfCiLGtTiPT1xUhFq9iLlPEZOKxEPOjIx7/KmmwX88VVifCjpV\n\t72oN5rD/41kMpdzpsMg0ZyLLlErGhB9LsL/V8y5EnSdoEQFZODgdRpKub6WHcZJirgKI\n\t/grt3XbjdpvbYoBIiN9wUvNLHIf7LjWrzxJ6SuK5XDSriLJXAbHzYeVOh0U+hZaPfE7p\n\tS9+vYRt+ImNNLmHVbc2iWyCrMlmeWHWmRXMp1J6MeSxVPqwmcoVQ+GkgiJHkOh3EW/qt\n\tfnQg==","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=FKWssiyrPfvNtQaelPVj9kFM55Cf4kWT8vePQJSEROc=;\n\tb=FhkN6LXs90i4u+714Y8J0rMrV6oIRya43LYQdgwet5WE3gE55LK947Xi4ZusamdraQ\n\tCsxgcuDK+UXI7lLtpCGKY5LZ8ykNWN++I8G4fR2dRaJyNMcK6ZuIZ0qVupdCqtni4CuH\n\tI6ma+o9bV0ZDWHjQjICb9nrBfvMmeK0KU6jQ8H8aYXGandekX9vCZ5UhU0HPWNWSinfa\n\tvq8///M37in9fMcVPEzRI2t2A2u6pZ+UHNOHk9B+D9jmyBlqZdydeH5TUuYa6rO1xXIG\n\tyOn0mjGNoHdqEaIwYH3V8GK0PVlVNQ23zVfmW4QlfD43b9l8sch36gKudSolT1K+4Rl+\n\tW96Q==","X-Gm-Message-State":"AOAM53050cXYMmZ46GdtPmzmhB02JH8mVtf5mty2j2Y+7fp1KgeahCuw\n\tGB1+R1VsE0DrM7HM3HkBmiJO5MBqj2CEopiruUiXYd1hscE=","X-Google-Smtp-Source":"ABdhPJxwslXkHLtdemTii5BEbxSCVs+4tF2hp4Duk67ORI/MugoifHbaS3PKEMs8W8RsVDtWHTrFR8x0KLrpMQFldLA=","X-Received":"by 2002:ac2:4adb:: with SMTP id m27mr9215221lfp.90.1595432005814;\n\tWed, 22 Jul 2020 08:33:25 -0700 (PDT)","MIME-Version":"1.0","References":"<20200720091311.805092-1-naush@raspberrypi.com>\n\t<20200720091311.805092-7-naush@raspberrypi.com>\n\t<4577acb0-3976-4721-6673-e456db4dc4b3@ideasonboard.com>\n\t\n\t<2a259e2a-9cb6-73c3-26f6-328a78077b48@ideasonboard.com>\n\t<20200722152513.GH29813@pendragon.ideasonboard.com>","In-Reply-To":"<20200722152513.GH29813@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Wed, 22 Jul 2020 16:33:08 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v4 6/9] libcamera: pipeline:\n\traspberrypi: Rework stream buffer logic for zero-copy","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11510,"web_url":"https://patchwork.libcamera.org/comment/11510/","msgid":"<20200722154827.GK29813@pendragon.ideasonboard.com>","date":"2020-07-22T15:48:27","subject":"Re: [libcamera-devel] [PATCH v4 6/9] libcamera: pipeline:\n\traspberrypi: Rework stream buffer logic for zero-copy","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Wed, Jul 22, 2020 at 04:33:08PM +0100, Naushir Patuck wrote:\n> On Wed, 22 Jul 2020 at 16:25, Laurent Pinchart wrote:\n> > On Wed, Jul 22, 2020 at 01:21:33PM +0100, Kieran Bingham wrote:\n> >> On 22/07/2020 09:04, Naushir Patuck wrote:\n> >>> On Tue, 21 Jul 2020 at 16:41, Kieran Bingham wrote:\n> >>>> On 20/07/2020 10:13, Naushir Patuck wrote:\n> >>>>> Stop using v4l2_videodevice::allocateBuffer() for internal buffers and\n> >>>>> instead export/import all buffers. This allows the pipeline to return\n> >>>>> any stream buffer requested by the application as zero-copy.\n> >>>>>\n> >>>>> Advertise the Unicam Image stream as the RAW capture stream now.\n> >>>>>\n> >>>>> The RPiStream object now maintains a new list of buffers that are\n> >>>>> available to queue into a device. This is needed to distinguish between\n> >>>>> FrameBuffers allocated for internal use vs externally provided buffers.\n> >>>>> When a Request comes in, if a buffer is not provided for an exported\n> >>>>> stream, we re-use a buffer from this list.\n> >>>>>\n> >>>>> Signed-off-by: Naushir Patuck \n> >>>>> Reviewed-by: Niklas Söderlund \n> >>>>\n> >>>> A few small nit fixups below but they could possibly be fixed while\n> >>>> applying anyway if you prefer.\n> >>>>\n> >>>> I've hit review fatigue though, so I'll look at this again tomorrow.\n> >>>\n> >>> I understand :)\n> >>> I will make the necessary fixups based on your review comments, and\n> >>> submit a new patch set. Will wait until you have finished reviewing\n> >>> this one before I send the next revision.\n> >>\n> >> Looking good, I think the only actionable thing remaining here is\n> >> potentially to use ASSERT() over assert() :-)\n> >>\n> >>>>> ---\n> >>>>> .../pipeline/raspberrypi/raspberrypi.cpp | 228 ++++++++++--------\n> >>>>> .../pipeline/raspberrypi/rpi_stream.cpp | 122 +++++++---\n> >>>>> .../pipeline/raspberrypi/rpi_stream.h | 30 ++-\n> >>>>> 3 files changed, 226 insertions(+), 154 deletions(-)\n> >>>>>\n> >>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>>>> index 8f6a999b..dbc22521 100644\n> >>>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>>>> @@ -187,7 +187,8 @@ private:\n> >>>>> void checkRequestCompleted();\n> >>>>> void tryRunPipeline();\n> >>>>> void tryFlushQueues();\n> >>>>> - FrameBuffer *updateQueue(std::queue &q, uint64_t timestamp, V4L2VideoDevice *dev);\n> >>>>> + FrameBuffer *updateQueue(std::queue &q, uint64_t timestamp,\n> >>>>> + RPi::RPiStream *stream);\n> >>>>>\n> >>>>> unsigned int ispOutputCount_;\n> >>>>> };\n> >>>>> @@ -508,8 +509,15 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> >>>>> StreamConfiguration &cfg = config->at(i);\n> >>>>>\n> >>>>> if (isRaw(cfg.pixelFormat)) {\n> >>>>> - cfg.setStream(&data->isp_[Isp::Input]);\n> >>>>> - data->isp_[Isp::Input].setExternal(true);\n> >>>>> + cfg.setStream(&data->unicam_[Unicam::Image]);\n> >>>>> + /*\n> >>>>> + * We must set both Unicam streams as external, even\n> >>>>> + * though the application may only request RAW frames.\n> >>>>> + * This is because we match timestamps on both streams\n> >>>>> + * to synchronise buffers.\n> >>>>> + */\n> >>>>> + data->unicam_[Unicam::Image].setExternal(true);\n> >>>>> + data->unicam_[Unicam::Embedded].setExternal(true);\n> >>>>> continue;\n> >>>>> }\n> >>>>>\n> >>>>> @@ -612,7 +620,7 @@ int PipelineHandlerRPi::exportFrameBuffers(Camera *camera, Stream *stream,\n> >>>>> unsigned int count = stream->configuration().bufferCount;\n> >>>>> int ret = s->dev()->exportBuffers(count, buffers);\n> >>>>>\n> >>>>> - s->setExternalBuffers(buffers);\n> >>>>> + s->setExportedBuffers(buffers);\n> >>>>>\n> >>>>> return ret;\n> >>>>> }\n> >>>>> @@ -712,14 +720,23 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> >>>>> if (data->state_ == RPiCameraData::State::Stopped)\n> >>>>> return -EINVAL;\n> >>>>>\n> >>>>> - /* Ensure all external streams have associated buffers! */\n> >>>>> - for (auto &stream : data->isp_) {\n> >>>>> - if (!stream.isExternal())\n> >>>>> - continue;\n> >>>>> + LOG(RPI, Debug) << \"queueRequestDevice: New request.\";\n> >>>>>\n> >>>>> - if (!request->findBuffer(&stream)) {\n> >>>>> - LOG(RPI, Error) << \"Attempt to queue request with invalid stream.\";\n> >>>>> - return -ENOENT;\n> >>>>> + /* Push all buffers supplied in the Request to the respective streams. */\n> >>>>> + for (auto stream : data->streams_) {\n> >>>>> + if (stream->isExternal()) {\n> >>>>\n> >>>> Are all streams now 'external' ?\n> >>>\n> >>> All streams but the ISP input (which only imports buffers) *can* be\n> >>> external. They are marked external only if the app configures the\n> >>> pipeline handler by saying it may request a buffer from the stream.\n> >>> In reality, all streams could be marked external all the time, but the\n> >>> buffer handling will take a slightly less efficient path, so I kept\n> >>> the distinction.\n> >>>\n> >>>>> + FrameBuffer *buffer = request->findBuffer(stream);\n> >>>>> + /*\n> >>>>> + * A nullptr in buffer means that we should queue an internally\n> >>>>> + * allocated buffer.\n> >>\n> >> Now I understand what's going on here, perhaps this could be expanded a\n> >> bit to explain?\n> >>\n> >> \"If no buffer is provided by the request for this stream, we queue a\n> >> nullptr to the stream to signify that it must use an internally\n> >> allocated buffer for this capture request which will not be given back\n> >> to the application, but can be used to support the internal pipeline flow.\"\n> >>\n> >> Now I see how this leads towards zero-copy raw support ;-)\n> >>\n> >>>>> + *\n> >>>>> + * The below queueBuffer() call will do nothing if there are not\n> >>>>> + * enough internal buffers allocated, but this will be handled by\n> >>>>> + * queuing the request for buffers in the RPiStream object.\n> >>>>> + */\n> >>>>> + int ret = stream->queueBuffer(buffer);\n> >>>>> + if (ret)\n> >>>>> + return ret;\n> >>>>> }\n> >>>>> }\n> >>>>>\n> >>>>> @@ -808,12 +825,10 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> >>>>> /* Initialize the camera properties. */\n> >>>>> data->properties_ = data->sensor_->properties();\n> >>>>>\n> >>>>> - /*\n> >>>>> - * List the available output streams.\n> >>>>> - * Currently cannot do Unicam streams!\n> >>>>> - */\n> >>>>> + /*List the available streams an application may request. */\n> >>>>\n> >>>> Minor nit, there's a space missing between /* and List ...\n> >>>>\n> >>>>> std::set streams;\n> >>>>> - streams.insert(&data->isp_[Isp::Input]);\n> >>>>> + streams.insert(&data->unicam_[Unicam::Image]);\n> >>>>> + streams.insert(&data->unicam_[Unicam::Embedded]);\n> >\n> > Can an application request the embedded data ?\n> \n> With this change, we can now advertise embedded data as well as isp\n> statistics available for the application to request. The only thing\n> missing is a way for the application to do so.\n\nCould we avoid exposing non-image streams to applications in the\nmeantime though ?\n\n> In a separate discussion we talked about libcamera::PixelFormat not\n> being the right place as these are data formats, not image formats.\n> No conclusion has been reached yet, but I will resurrect that\n> discussion shortly :-)\n\nTake your time :-) Jokes aside, I'm working on support for\nmemory-to-memory processing (a.k.a. reprocessing), and I think that\nshould be merged first as a base to provide statistics to applications.\nI expect it will take at least a couple of months until the reprocessing\nAPI is ready, so there's no hurry at all.\n\n> >>>>> streams.insert(&data->isp_[Isp::Output0]);\n> >>>>> streams.insert(&data->isp_[Isp::Output1]);\n> >>>>> streams.insert(&data->isp_[Isp::Stats]);\n> >>>>> @@ -831,9 +846,28 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> >>>>> int ret;\n> >>>>>\n> >>>>> for (auto const stream : data->streams_) {\n> >>>>> - ret = stream->queueBuffers();\n> >>>>> - if (ret < 0)\n> >>>>> - return ret;\n> >>>>> + if (!stream->isExternal()) {\n> >>>>> + ret = stream->queueAllBuffers();\n> >>>>> + if (ret < 0)\n> >>>>> + return ret;\n> >>>>> + } else {\n> >>>>> + /*\n> >>>>> + * For external streams, we must queue up a set of internal\n> >>>>> + * buffers to handle the number of drop frames requested by\n> >>>>> + * the IPA. This is done by passing nullptr in queueBuffer().\n> >>>>> + *\n> >>>>> + * The below queueBuffer() call will do nothing if there\n> >>>>> + * are not enough internal buffers allocated, but this will\n> >>>>> + * be handled by queuing the request for buffers in the\n> >>>>> + * RPiStream object.\n> >>>>> + */\n> >>>>> + unsigned int i;\n> >>>>> + for (i = 0; i < data->dropFrameCount_; i++) {\n> >>>>> + int ret = stream->queueBuffer(nullptr);\n> >>>>> + if (ret)\n> >>>>> + return ret;\n> >>>>> + }\n> >>>>> + }\n> >>>>> }\n> >>>>>\n> >>>>> return 0;\n> >>>>> @@ -847,7 +881,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> >>>>> /*\n> >>>>> * Decide how many internal buffers to allocate. For now, simply look\n> >>>>> * at how many external buffers will be provided. Will need to improve\n> >>>>> - * this logic.\n> >>>>> + * this logic. However, we really must have all stream allocate the same\n> >>>>\n> >>>> s/stream/streams/\n> >>>>\n> >>>>> + * number of buffers to simplify error handling in queueRequestDevice().\n> >>>>> */\n> >>>>\n> >>>> Does this include Raw streams? I thought that allocates less buffers? Or\n> >>>> perhaps it's the same number of internal buffers, and only 2 extra\n> >>>> external buffers for that case...\n> >>>\n> >>> That's correct. For now all (including Raw) streams allocate the same\n> >>> number of internal buffers. Hence the comment that we could improve\n> >>> this logic, it is a bit wasteful on memory. This bit does need\n> >>> refinement at some point.\n> >>>\n> >>>>> unsigned int maxBuffers = 0;\n> >>>>> for (const Stream *s : camera->streams())\n> >>>>> @@ -855,33 +890,9 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> >>>>> maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);\n> >>>>>\n> >>>>> for (auto const stream : data->streams_) {\n> >>>>> - if (stream->isExternal() || stream->isImporter()) {\n> >>>>> - /*\n> >>>>> - * If a stream is marked as external reserve memory to\n> >>>>> - * prepare to import as many buffers are requested in\n> >>>>> - * the stream configuration.\n> >>>>> - *\n> >>>>> - * If a stream is an internal stream with importer\n> >>>>> - * role, reserve as many buffers as possible.\n> >>>>> - */\n> >>>>> - unsigned int count = stream->isExternal()\n> >>>>> - ? stream->configuration().bufferCount\n> >>>>> - : maxBuffers;\n> >>>>> - ret = stream->importBuffers(count);\n> >>>>> - if (ret < 0)\n> >>>>> - return ret;\n> >>>>> - } else {\n> >>>>> - /*\n> >>>>> - * If the stream is an internal exporter allocate and\n> >>>>> - * export as many buffers as possible to its internal\n> >>>>> - * pool.\n> >>>>> - */\n> >>>>> - ret = stream->allocateBuffers(maxBuffers);\n> >>>>> - if (ret < 0) {\n> >>>>> - freeBuffers(camera);\n> >>>>> - return ret;\n> >>>>> - }\n> >>>>> - }\n> >>>>> + ret = stream->prepareBuffers(maxBuffers);\n> >>>>> + if (ret < 0)\n> >>>>> + return ret;\n> >\n> > This doesn't free buffers anymore on failure, is that an oversight ?\n> \n> Oversight. Will fix.\n> \n> >\n> >>>>> }\n> >>>>>\n> >>>>> /*\n> >>>>> @@ -889,7 +900,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> >>>>> * the IPA and RPI_IPA_EVENT_SIGNAL_ISP_PREPARE event.\n> >>>>> */\n> >>>>> count = 0;\n> >>>>> - for (auto const &b : *data->unicam_[Unicam::Image].getBuffers()) {\n> >>>>> + for (auto const &b : data->unicam_[Unicam::Image].getBuffers()) {\n> >>>>> b->setCookie(count++);\n> >>>>> }\n> >>>>>\n> >>>>> @@ -898,14 +909,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> >>>>> * the IPA.\n> >>>>> */\n> >>>>> count = 0;\n> >>>>> - for (auto const &b : *data->isp_[Isp::Stats].getBuffers()) {\n> >>>>> + for (auto const &b : data->isp_[Isp::Stats].getBuffers()) {\n> >>>>> b->setCookie(count++);\n> >>>>> data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b->cookie(),\n> >>>>> .planes = b->planes() });\n> >>>>> }\n> >>>>>\n> >>>>> count = 0;\n> >>>>> - for (auto const &b : *data->unicam_[Unicam::Embedded].getBuffers()) {\n> >>>>> + for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {\n> >>>>> b->setCookie(count++);\n> >>>>> data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b->cookie(),\n> >>>>> .planes = b->planes() });\n> >>>>> @@ -1066,7 +1077,7 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n> >>>>> switch (action.operation) {\n> >>>>> case RPI_IPA_ACTION_STATS_METADATA_COMPLETE: {\n> >>>>> unsigned int bufferId = action.data[0];\n> >>>>> - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers()->at(bufferId).get();\n> >>>>> + FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> >>>>>\n> >>>>> handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> >>>>> /* Fill the Request metadata buffer with what the IPA has provided */\n> >>>>> @@ -1077,19 +1088,19 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n> >>>>>\n> >>>>> case RPI_IPA_ACTION_EMBEDDED_COMPLETE: {\n> >>>>> unsigned int bufferId = action.data[0];\n> >>>>> - FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers()->at(bufferId).get();\n> >>>>> + FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);\n> >>>>> handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> >>>>> break;\n> >>>>> }\n> >>>>>\n> >>>>> case RPI_IPA_ACTION_RUN_ISP: {\n> >>>>> unsigned int bufferId = action.data[0];\n> >>>>> - FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers()->at(bufferId).get();\n> >>>>> + FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);\n> >>>>>\n> >>>>> LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << buffer->cookie()\n> >>>>> << \", timestamp: \" << buffer->metadata().timestamp;\n> >>>>>\n> >>>>> - isp_[Isp::Input].dev()->queueBuffer(buffer);\n> >>>>> + isp_[Isp::Input].queueBuffer(buffer);\n> >>>>> ispOutputCount_ = 0;\n> >>>>> break;\n> >>>>> }\n> >>>>> @@ -1190,22 +1201,27 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n> >>>>> << \", buffer id \" << buffer->cookie()\n> >>>>> << \", timestamp: \" << buffer->metadata().timestamp;\n> >>>>>\n> >>>>> - handleStreamBuffer(buffer, stream);\n> >>>>> -\n> >>>>> /*\n> >>>>> - * Increment the number of ISP outputs generated.\n> >>>>> - * This is needed to track dropped frames.\n> >>>>> + * ISP statistics buffer must not be re-queued or sent back to the\n> >>>>> + * application until after the IPA signals so.\n> >>>>> */\n> >>>>> - ispOutputCount_++;\n> >>>>> -\n> >>>>> - /* If this is a stats output, hand it to the IPA now. */\n> >>>>> if (stream == &isp_[Isp::Stats]) {\n> >>>>> IPAOperationData op;\n> >>>>> op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;\n> >>>>> op.data = { RPiIpaMask::STATS | buffer->cookie() };\n> >>>>> ipa_->processEvent(op);\n> >>>>> + } else {\n> >>>>> + /* Any other ISP output can be handed back to the application now. */\n> >>>>> + handleStreamBuffer(buffer, stream);\n> >>>>> }\n> >>>>>\n> >>>>> + /*\n> >>>>> + * Increment the number of ISP outputs generated.\n> >>>>> + * This is needed to track dropped frames.\n> >>>>> + */\n> >>>>> + ispOutputCount_++;\n> >>>>> +\n> >>>>> +\n> >\n> > Extra blank line ?\n> >\n> >>>>> handleState();\n> >>>>> }\n> >>>>>\n> >>>>> @@ -1218,8 +1234,12 @@ void RPiCameraData::clearIncompleteRequests()\n> >>>>> */\n> >>>>> for (auto const request : requestQueue_) {\n> >>>>> for (auto const stream : streams_) {\n> >>>>> - if (stream->isExternal())\n> >>>>> - stream->dev()->queueBuffer(request->findBuffer(stream));\n> >>>>> + if (!stream->isExternal())\n> >>>>> + continue;\n> >>>>> +\n> >>>>> + FrameBuffer *buffer = request->findBuffer(stream);\n> >>>>> + if (buffer)\n> >>>>> + stream->queueBuffer(buffer);\n> >>>>> }\n> >>>>> }\n> >>>>>\n> >>>>> @@ -1248,7 +1268,7 @@ void RPiCameraData::clearIncompleteRequests()\n> >>>>> * Has the buffer already been handed back to the\n> >>>>> * request? If not, do so now.\n> >>>>> */\n> >>>>> - if (buffer->request())\n> >>>>> + if (buffer && buffer->request())\n> >>>>> pipe_->completeBuffer(camera_, request, buffer);\n> >>>>> }\n> >>>>>\n> >>>>> @@ -1260,30 +1280,24 @@ void RPiCameraData::clearIncompleteRequests()\n> >>>>> void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)\n> >>>>> {\n> >>>>> if (stream->isExternal()) {\n> >>>>> - if (!dropFrameCount_) {\n> >>>>> - Request *request = buffer->request();\n> >>>>> + Request *request = requestQueue_.front();\n> >>>>> +\n> >>>>> + if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {\n> >>>>> + /*\n> >>>>> + * Tag the buffer as completed, returning it to the\n> >>>>> + * application.\n> >>>>> + */\n> >>>>> pipe_->completeBuffer(camera_, request, buffer);\n> >>>>> + } else {\n> >>>>> + /*\n> >>>>> + * This buffer was not part of the Request, so we can\n> >>>>> + * recycle it.\n> >>>>> + */\n> >>>>> + stream->returnBuffer(buffer);\n> >>>>> }\n> >>>>> } else {\n> >>>>> - /* Special handling for RAW buffer Requests.\n> >>>>> - *\n> >>>>> - * The ISP input stream is alway an import stream, but if the\n> >>>>> - * current Request has been made for a buffer on the stream,\n> >>>>> - * simply memcpy to the Request buffer and requeue back to the\n> >>>>> - * device.\n> >>>>> - */\n> >>>>> - if (stream == &unicam_[Unicam::Image] && !dropFrameCount_) {\n> >>>>> - const Stream *rawStream = static_cast(&isp_[Isp::Input]);\n> >>>>> - Request *request = requestQueue_.front();\n> >>>>> - FrameBuffer *raw = request->findBuffer(const_cast(rawStream));\n> >>>>> - if (raw) {\n> >>>>> - raw->copyFrom(buffer);\n> >>>>> - pipe_->completeBuffer(camera_, request, raw);\n> >>>>> - }\n> >>>>> - }\n> >>>>> -\n> >>>>> - /* Simply requeue the buffer. */\n> >>>>> - stream->dev()->queueBuffer(buffer);\n> >>>>> + /* Simply re-queue the buffer to the requested stream. */\n> >>>>> + stream->queueBuffer(buffer);\n> >>>>> }\n> >>>>> }\n> >>>>>\n> >>>>> @@ -1367,7 +1381,7 @@ void RPiCameraData::tryRunPipeline()\n> >>>>> * current bayer buffer will be removed and re-queued to the driver.\n> >>>>> */\n> >>>>> embeddedBuffer = updateQueue(embeddedQueue_, bayerBuffer->metadata().timestamp,\n> >>>>> - unicam_[Unicam::Embedded].dev());\n> >>>>> + &unicam_[Unicam::Embedded]);\n> >>>>>\n> >>>>> if (!embeddedBuffer) {\n> >>>>> LOG(RPI, Debug) << \"Could not find matching embedded buffer\";\n> >>>>> @@ -1386,7 +1400,7 @@ void RPiCameraData::tryRunPipeline()\n> >>>>>\n> >>>>> embeddedBuffer = embeddedQueue_.front();\n> >>>>> bayerBuffer = updateQueue(bayerQueue_, embeddedBuffer->metadata().timestamp,\n> >>>>> - unicam_[Unicam::Image].dev());\n> >>>>> + &unicam_[Unicam::Image]);\n> >>>>>\n> >>>>> if (!bayerBuffer) {\n> >>>>> LOG(RPI, Debug) << \"Could not find matching bayer buffer - ending.\";\n> >>>>> @@ -1394,11 +1408,7 @@ void RPiCameraData::tryRunPipeline()\n> >>>>> }\n> >>>>> }\n> >>>>>\n> >>>>> - /*\n> >>>>> - * Take the first request from the queue and action the IPA.\n> >>>>> - * Unicam buffers for the request have already been queued as they come\n> >>>>> - * in.\n> >>>>> - */\n> >>>>> + /* Take the first request from the queue and action the IPA. */\n> >>>>> Request *request = requestQueue_.front();\n> >>>>>\n> >>>>> /*\n> >>>>> @@ -1410,12 +1420,6 @@ void RPiCameraData::tryRunPipeline()\n> >>>>> op.controls = { request->controls() };\n> >>>>> ipa_->processEvent(op);\n> >>>>>\n> >>>>> - /* Queue up any ISP buffers passed into the request. */\n> >>>>> - for (auto &stream : isp_) {\n> >>>>> - if (stream.isExternal())\n> >>>>> - stream.dev()->queueBuffer(request->findBuffer(&stream));\n> >>>>> - }\n> >>>>> -\n> >>>>> /* Ready to use the buffers, pop them off the queue. */\n> >>>>> bayerQueue_.pop();\n> >>>>> embeddedQueue_.pop();\n> >>>>> @@ -1445,32 +1449,42 @@ void RPiCameraData::tryFlushQueues()\n> >>>>> * and give a chance for the hardware to return to lock-step. We do have\n> >>>>> * to drop all interim frames.\n> >>>>> */\n> >>>>> - if (unicam_[Unicam::Image].getBuffers()->size() == bayerQueue_.size() &&\n> >>>>> - unicam_[Unicam::Embedded].getBuffers()->size() == embeddedQueue_.size()) {\n> >>>>> + if (unicam_[Unicam::Image].getBuffers().size() == bayerQueue_.size() &&\n> >>>>> + unicam_[Unicam::Embedded].getBuffers().size() == embeddedQueue_.size()) {\n> >>>>> + /* This cannot happen when Unicam streams are external. */\n> >>>>> + assert(!unicam_[Unicam::Image].isExternal());\n> >>>>> +\n> >>>>> LOG(RPI, Warning) << \"Flushing all buffer queues!\";\n> >>>>>\n> >>>>> while (!bayerQueue_.empty()) {\n> >>>>> - unicam_[Unicam::Image].dev()->queueBuffer(bayerQueue_.front());\n> >>>>> + unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());\n> >>>>> bayerQueue_.pop();\n> >>>>> }\n> >>>>>\n> >>>>> while (!embeddedQueue_.empty()) {\n> >>>>> - unicam_[Unicam::Embedded].dev()->queueBuffer(embeddedQueue_.front());\n> >>>>> + unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());\n> >>>>> embeddedQueue_.pop();\n> >>>>> }\n> >>>>> }\n> >>>>> }\n> >>>>>\n> >>>>> FrameBuffer *RPiCameraData::updateQueue(std::queue &q, uint64_t timestamp,\n> >>>>> - V4L2VideoDevice *dev)\n> >>>>> + RPi::RPiStream *stream)\n> >>>>> {\n> >>>>> + /*\n> >>>>> + * If the unicam streams are external (both have to the same), then we\n> >>>>\n> >>>> both have to 'be' the same?\n> >>>>\n> >>>>> + * can only return out the top buffer in the queue, and assume they have\n> >>>>> + * been synced by queuing at the same time. We cannot drop these frames,\n> >>>>> + * as they may have been provided externally.\n> >>>>> + */\n> >>>>> while (!q.empty()) {\n> >>>>> FrameBuffer *b = q.front();\n> >>>>> - if (b->metadata().timestamp < timestamp) {\n> >>>>> + if (!stream->isExternal() && b->metadata().timestamp < timestamp) {\n> >>>>> q.pop();\n> >>>>> - dev->queueBuffer(b);\n> >>>>> - LOG(RPI, Warning) << \"Dropping input frame!\";\n> >>>>> - } else if (b->metadata().timestamp == timestamp) {\n> >>>>> + stream->queueBuffer(b);\n> >>>>> + LOG(RPI, Warning) << \"Dropping unmatched input frame in stream \"\n> >>>>> + << stream->name();\n> >>>>> + } else if (stream->isExternal() || b->metadata().timestamp == timestamp) {\n> >>>>> /* The calling function will pop the item from the queue. */\n> >>>>> return b;\n> >>>>> } else {\n> >>>>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> >>>>> index 2edb8b59..02f8d3e0 100644\n> >>>>> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> >>>>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> >>>>> @@ -21,30 +21,20 @@ V4L2VideoDevice *RPiStream::dev() const\n> >>>>>\n> >>>>> void RPiStream::setExternal(bool external)\n> >>>>> {\n> >>>>> + /* Import streams cannot be external. */\n> >>>>> + assert(!external || !importOnly_);\n> >>>>> external_ = external;\n> >>>>> }\n> >>>>>\n> >>>>> bool RPiStream::isExternal() const\n> >>>>> {\n> >>>>> - /*\n> >>>>> - * Import streams cannot be external.\n> >>>>> - *\n> >>>>> - * RAW capture is a special case where we simply copy the RAW\n> >>>>> - * buffer out of the request. All other buffer handling happens\n> >>>>> - * as if the stream is internal.\n> >>>>> - */\n> >>>>> - return external_ && !importOnly_;\n> >>>>> -}\n> >>>>> -\n> >>>>> -bool RPiStream::isImporter() const\n> >>>>> -{\n> >>>>> - return importOnly_;\n> >>>>> + return external_;\n> >>>>> }\n> >>>>>\n> >>>>> void RPiStream::reset()\n> >>>>> {\n> >>>>> external_ = false;\n> >>>>> - internalBuffers_.clear();\n> >>>>> + clearBuffers();\n> >>>>> }\n> >>>>>\n> >>>>> std::string RPiStream::name() const\n> >>>>> @@ -52,65 +42,123 @@ std::string RPiStream::name() const\n> >>>>> return name_;\n> >>>>> }\n> >>>>>\n> >>>>> -void RPiStream::setExternalBuffers(std::vector> *buffers)\n> >>>>> +void RPiStream::setExportedBuffers(std::vector> *buffers)\n> >>>>> {\n> >>>>> - externalBuffers_ = buffers;\n> >>>>> + std::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_),\n> >>>>> + [](std::unique_ptr &b) { return b.get(); });\n> >>>>> }\n> >>>>>\n> >>>>> -const std::vector> *RPiStream::getBuffers() const\n> >>>>> +const std::vector &RPiStream::getBuffers() const\n> >>>>> {\n> >>>>> - return external_ ? externalBuffers_ : &internalBuffers_;\n> >>>>> + return bufferList_;\n> >>>>> }\n> >>>>>\n> >>>>> void RPiStream::releaseBuffers()\n> >>>>> {\n> >>>>> dev_->releaseBuffers();\n> >>>>> - if (!external_ && !importOnly_)\n> >>>>> - internalBuffers_.clear();\n> >>>>> + clearBuffers();\n> >>>>> }\n> >>>>>\n> >>>>> -int RPiStream::importBuffers(unsigned int count)\n> >>>>> +int RPiStream::prepareBuffers(unsigned int count)\n> >>>>> {\n> >>>>> + int ret;\n> >>>>> +\n> >>>>> + if (!importOnly_) {\n> >>>>> + if (count) {\n> >>>>> + /* Export some frame buffers for internal use. */\n> >>>>> + ret = dev_->exportBuffers(count, &internalBuffers_);\n> >>>>> + if (ret < 0)\n> >>>>> + return ret;\n> >>>>> +\n> >>>>> + /* Add these exported buffers to the internal/external buffer list. */\n> >>>>> + setExportedBuffers(&internalBuffers_);\n> >>>>> +\n> >>>>> + /* Add these buffers to the queue of internal usable buffers. */\n> >>>>> + for (auto const &buffer : internalBuffers_)\n> >>>>> + availableBuffers_.push(buffer.get());\n> >>>>> + }\n> >>>>> +\n> >>>>> + /* We must import all internal/external exported buffers. */\n> >>>>> + count = bufferList_.size();\n> >>>>> + }\n> >>>>> +\n> >>>>> return dev_->importBuffers(count);\n> >>>>> }\n> >>>>>\n> >>>>> -int RPiStream::allocateBuffers(unsigned int count)\n> >>>>> +int RPiStream::queueAllBuffers()\n> >>>>> {\n> >>>>> - return dev_->allocateBuffers(count, &internalBuffers_);\n> >>>>> -}\n> >>>>> + int ret;\n> >>>>>\n> >>>>> -int RPiStream::queueBuffers()\n> >>>>> -{\n> >>>>> if (external_)\n> >>>>> return 0;\n> >>>>>\n> >>>>> - for (auto &b : internalBuffers_) {\n> >>>>> - int ret = dev_->queueBuffer(b.get());\n> >>>>> - if (ret) {\n> >>>>> - LOG(RPISTREAM, Error) << \"Failed to queue buffers for \"\n> >>>>> - << name_;\n> >>>>> + while (!availableBuffers_.empty()) {\n> >>>>> + ret = queueBuffer(availableBuffers_.front());\n> >>>>> + if (ret < 0)\n> >>>>> return ret;\n> >>>>> - }\n> >>>>> +\n> >>>>> + availableBuffers_.pop();\n> >>>>> }\n> >>>>>\n> >>>>> return 0;\n> >>>>> }\n> >>>>>\n> >>>>> -bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const\n> >>>>> +int RPiStream::queueBuffer(FrameBuffer *buffer)\n> >>>>> +{\n> >>>>> + /*\n> >>>>> + * A nullptr buffer implies an external stream, but no external\n> >>>>> + * buffer has been supplied. So, pick one from the availableBuffers_\n> >>>>> + * queue.\n> >>\n> >> Aha, now I see this here, maybe the comment addition I suggested above\n> >> is possibly redundant, as it's explained here ...\n> >>\n> >>>>> + */\n> >>>>> + if (!buffer) {\n> >>>>> + if (availableBuffers_.empty()) {\n> >>>>> + LOG(RPISTREAM, Warning) << \"No buffers available for \"\n> >>>>> + << name_;\n> >>>>> + return -EINVAL;\n> >>>>> + }\n> >>>>> +\n> >>>>> + buffer = availableBuffers_.front();\n> >>>>> + availableBuffers_.pop();\n> >>>>> + }\n> >>>>> +\n> >>>>> + LOG(RPISTREAM, Debug) << \"Queuing buffer \" << buffer->cookie()\n> >>>>> + << \" for \" << name_;\n> >>>>> +\n> >>>>> + int ret = dev_->queueBuffer(buffer);\n> >>>>> + if (ret) {\n> >>>>> + LOG(RPISTREAM, Error) << \"Failed to queue buffer for \"\n> >>>>> + << name_;\n> >>>>> + }\n> >>>>> +\n> >>>>> + return ret;\n> >>>>> +}\n> >>>>> +\n> >>>>> +void RPiStream::returnBuffer(FrameBuffer *buffer)\n> >>>>> {\n> >>>>> - auto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();\n> >>>>> - auto end = external_ ? externalBuffers_->end() : internalBuffers_.end();\n> >>>>> + /* This can only be called for external streams. */\n> >>>>> + assert(external_);\n> >>\n> >> Could this use ASSERT() from libcamera/internal/log.h ?\n> >>\n> >>>>> +\n> >>>>> + availableBuffers_.push(buffer);\n> >>>>> +}\n> >>>>>\n> >>>>> +bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const\n> >>>>> +{\n> >>>>> if (importOnly_)\n> >>>>> return false;\n> >>>>>\n> >>>>> - if (std::find_if(start, end,\n> >>>>> - [buffer](std::unique_ptr const &ref) { return ref.get() == buffer; }) != end)\n> >>>>> + if (std::find(bufferList_.begin(), bufferList_.end(), buffer) != bufferList_.end())\n> >>>>> return true;\n> >>>>>\n> >>>>> return false;\n> >>>>> }\n> >>>>>\n> >>>>> +void RPiStream::clearBuffers()\n> >>>>> +{\n> >>>>> + availableBuffers_ = std::queue{};\n> >>>>> + internalBuffers_.clear();\n> >>>>> + bufferList_.clear();\n> >>>>> +}\n> >>>>> +\n> >>>>> } /* namespace RPi */\n> >>>>>\n> >>>>> } /* namespace libcamera */\n> >>>>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> >>>>> index 3957e342..af9c2ad2 100644\n> >>>>> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> >>>>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> >>>>> @@ -39,21 +39,22 @@ public:\n> >>>>> V4L2VideoDevice *dev() const;\n> >>>>> void setExternal(bool external);\n> >>>>> bool isExternal() const;\n> >>>>> - bool isImporter() const;\n> >>>>> void reset();\n> >>>>> std::string name() const;\n> >>>>> - void setExternalBuffers(std::vector> *buffers);\n> >>>>> - const std::vector> *getBuffers() const;\n> >>>>> + void setExportedBuffers(std::vector> *buffers);\n> >>>>> + const std::vector &getBuffers() const;\n> >>>>> void releaseBuffers();\n> >>>>> - int importBuffers(unsigned int count);\n> >>>>> - int allocateBuffers(unsigned int count);\n> >>>>> - int queueBuffers();\n> >>>>> + int prepareBuffers(unsigned int count);\n> >>>>> + int queueAllBuffers();\n> >>>>> + int queueBuffer(FrameBuffer *buffer);\n> >>>>> + void returnBuffer(FrameBuffer *buffer);\n> >>>>> bool findFrameBuffer(FrameBuffer *buffer) const;\n> >>>>\n> >>>> Perhaps we should break those up into sections in patch 1/9. It's hard\n> >>>> to see the wood-for-the-trees in the wall of function declarations.\n> >>>\n> >>> Not sure I follow what you mean?\n> >>\n> >> There is a lot of text all bunched up together, so it's hard to get my\n> >> eyes around the class members.\n> >>\n> >> It would be nice to group functions with some line breaks to make it\n> >> easier on the eye.\n> >>\n> >> But that should happen in patch 1/9, so I'll go comment on it there. I\n> >> skipped it before because that patch just moves code around, but I thnik\n> >> it can also improve formatting for readability.\n> >>\n> >>>>>\n> >>>>> private:\n> >>>>> + void clearBuffers();\n> >>>>> /*\n> >>>>> * Indicates that this stream is active externally, i.e. the buffers\n> >>>>> - * are provided by the application.\n> >>>>> + * might be provided by (and returned to) the application.\n> >>>>> */\n> >>>>> bool external_;\n> >>>>> /* Indicates that this stream only imports buffers, e.g. ISP input. */\n> >>>>> @@ -62,10 +63,19 @@ private:\n> >>>>> std::string name_;\n> >>>>> /* The actual device stream. */\n> >>>>> std::unique_ptr dev_;\n> >>>>> - /* Internally allocated framebuffers associated with this device stream. */\n> >>>>> + /* All framebuffers associated with this device stream. */\n> >>>>> + std::vector bufferList_;\n> >>>>> + /*\n> >>>>> + * List of frame buffer that we can use if none have been provided by\n> >>>>> + * the application for external streams. This is populated by the\n> >>>>> + * buffers exported internally.\n> >>>>> + */\n> >>>>> + std::queue availableBuffers_;\n> >>>>> + /*\n> >>>>> + * This is a list of buffers exported internally. Need to keep this around\n> >>>>> + * as the stream needs to maintain ownership of these buffers.\n> >>>>> + */\n> >>>>> std::vector> internalBuffers_;\n> >>>>> - /* Externally allocated framebuffers associated with this device stream. */\n> >>>>> - std::vector> *externalBuffers_;\n> >>>>\n> >>>> Likewise here, some whitespace to break up these members would really\n> >>>> help, as by the time I've got here I've got definite review fatigue and\n> >>>> lots of lines close together just become a blur ... :S\n> >>>\n> >>> That's not a problem.\n> >>\n> >> But for this patch, I can't see anything wrong ... and I can see how\n> >> it's helping manage buffers to allow optional buffers on a stream for\n> >> RAW zero copy support.\n> >>\n> >> Reviewed-by: Kieran Bingham \n> >>\n> >>>>> };\n> >>>>>\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 60F95BDB1B\n\tfor ;\n\tWed, 22 Jul 2020 15:48:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EBC6D60C34;\n\tWed, 22 Jul 2020 17:48: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 BEC5960540\n\tfor ;\n\tWed, 22 Jul 2020 17:48:35 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 60EB6329;\n\tWed, 22 Jul 2020 17:48:32 +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=\"SvjQ/kO0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1595432912;\n\tbh=ZlFNIfzwISBDDyQ3FkU613gkV2lsS1eSoEG9U0Oelyw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SvjQ/kO0qqFl2w1G/C8iwsaEvaJbaRLeND1yNc9xfFzeyHzoVZpPtiXrrijBao0/W\n\tPi7y2r4jXwHSz5uR1puHXZiY5ZcJcDU+sflGez8cnTdUnaM5ozm/9vgy55uLIsaMWk\n\tWBY76Fp6+Q7Aq4Fo4qqmBjs80UaALjh/9gzuXhHQ=","Date":"Wed, 22 Jul 2020 18:48:27 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"<20200722154827.GK29813@pendragon.ideasonboard.com>","References":"<20200720091311.805092-1-naush@raspberrypi.com>\n\t<20200720091311.805092-7-naush@raspberrypi.com>\n\t<4577acb0-3976-4721-6673-e456db4dc4b3@ideasonboard.com>\n\t\n\t<2a259e2a-9cb6-73c3-26f6-328a78077b48@ideasonboard.com>\n\t<20200722152513.GH29813@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 6/9] libcamera: pipeline:\n\traspberrypi: Rework stream buffer logic for zero-copy","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]