[{"id":12474,"web_url":"https://patchwork.libcamera.org/comment/12474/","msgid":"<20200912151035.GD695456@oden.dyn.berto.se>","date":"2020-09-12T15:10:35","subject":"Re: [libcamera-devel] [PATCH v7 12/12] pipeline: ipa: raspberrypi:\n\tHandle any externally allocated FrameBuffer","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Naushir,\n\nThanks for your patch.\n\nOn 2020-09-08 08:49:12 +0100, Naushir Patuck wrote:\n> Handle the case where a FrameBuffer that has been externally allocated\n> (i.e. not through the v4l2 video device) is passed into a Request.\n> \n> We must store the buffer pointer in the stream internal buffer list to\n> identify when used.\n> \n> Signed-off-by: Naushir Patuck \n\nReviewed-by: Niklas Söderlund \n\n> ---\n> include/libcamera/ipa/raspberrypi.h | 11 ++--\n> src/ipa/raspberrypi/raspberrypi.cpp | 6 +-\n> .../pipeline/raspberrypi/raspberrypi.cpp | 64 +++++++++++++------\n> .../pipeline/raspberrypi/rpi_stream.cpp | 30 ++++++++-\n> .../pipeline/raspberrypi/rpi_stream.h | 3 +\n> 5 files changed, 85 insertions(+), 29 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index 262fc6f3..dd6ebeac 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -28,11 +28,12 @@ enum RPiOperations {\n> \tRPI_IPA_EVENT_QUEUE_REQUEST,\n> };\n> \n> -enum RPiIpaMask {\n> -\tID\t\t= 0x0ffff,\n> -\tSTATS\t\t= 0x10000,\n> -\tEMBEDDED_DATA\t= 0x20000,\n> -\tBAYER_DATA\t= 0x40000\n> +enum RPiBufferMask {\n> +\tID\t\t= 0x00ffff,\n> +\tSTATS\t\t= 0x010000,\n> +\tEMBEDDED_DATA\t= 0x020000,\n> +\tBAYER_DATA\t= 0x040000,\n> +\tEXTERNAL_BUFFER\t= 0x100000,\n> };\n> \n> /* Size of the LS grid allocation. */\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 688d2efc..0555cc4e 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -352,7 +352,7 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> \n> \t\tIPAOperationData op;\n> \t\top.operation = RPI_IPA_ACTION_STATS_METADATA_COMPLETE;\n> -\t\top.data = { bufferId & RPiIpaMask::ID };\n> +\t\top.data = { bufferId & RPiBufferMask::ID };\n> \t\top.controls = { libcameraMetadata_ };\n> \t\tqueueFrameAction.emit(0, op);\n> \t\tbreak;\n> @@ -373,7 +373,7 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> \t\t/* Ready to push the input buffer into the ISP. */\n> \t\tIPAOperationData op;\n> \t\top.operation = RPI_IPA_ACTION_RUN_ISP;\n> -\t\top.data = { bayerbufferId & RPiIpaMask::ID };\n> +\t\top.data = { bayerbufferId & RPiBufferMask::ID };\n> \t\tqueueFrameAction.emit(0, op);\n> \t\tbreak;\n> \t}\n> @@ -700,7 +700,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> {\n> \tIPAOperationData op;\n> \top.operation = RPI_IPA_ACTION_EMBEDDED_COMPLETE;\n> -\top.data = { bufferId & RPiIpaMask::ID };\n> +\top.data = { bufferId & RPiBufferMask::ID };\n> \tqueueFrameAction.emit(0, op);\n> }\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 5621f182..f5e0d1cd 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -151,6 +151,7 @@ public:\n> \n> \tvoid clearIncompleteRequests();\n> \tvoid handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);\n> +\tvoid handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);\n> \tvoid handleState();\n> \n> \tCameraSensor *sensor_;\n> @@ -725,23 +726,32 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> \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> -\t\t\tFrameBuffer *buffer = request->findBuffer(stream);\n> +\t\tif (!stream->isExternal())\n> +\t\t\tcontinue;\n> +\n> +\t\tFrameBuffer *buffer = request->findBuffer(stream);\n> +\t\tif (buffer && stream->getBufferId(buffer) == -1) {\n> \t\t\t/*\n> -\t\t\t * If no buffer is provided by the request for this stream, we\n> -\t\t\t * queue a nullptr to the stream to signify that it must use an\n> -\t\t\t * internally allocated buffer for this capture request. This\n> -\t\t\t * buffer will not be given back to the application, but is used\n> -\t\t\t * to support the internal pipeline flow.\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 * This buffer is not recognised, so it must have been allocated\n> +\t\t\t * outside the v4l2 device. Store it in the stream buffer list\n> +\t\t\t * so we can track it.\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\tstream->setExternalBuffer(buffer);\n> \t\t}\n> +\t\t/*\n> +\t\t * If no buffer is provided by the request for this stream, we\n> +\t\t * queue a nullptr to the stream to signify that it must use an\n> +\t\t * internally allocated buffer for this capture request. This\n> +\t\t * buffer will not be given back to the application, but is used\n> +\t\t * to support the internal pipeline flow.\n> +\t\t *\n> +\t\t * The below queueBuffer() call will do nothing if there are not\n> +\t\t * enough internal buffers allocated, but this will be handled by\n> +\t\t * queuing the request for buffers in the RPiStream object.\n> +\t\t */\n> +\t\tint ret = stream->queueBuffer(buffer);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> \t}\n> \n> \t/* Push the request to the back of the queue. */\n> @@ -915,8 +925,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> \t * Pass the stats and embedded data buffers to the IPA. No other\n> \t * buffers need to be passed.\n> \t */\n> -\tmapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPiIpaMask::STATS);\n> -\tmapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), RPiIpaMask::EMBEDDED_DATA);\n> +\tmapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPiBufferMask::STATS);\n> +\tmapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), RPiBufferMask::EMBEDDED_DATA);\n> \n> \treturn 0;\n> }\n> @@ -1219,7 +1229,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\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 | static_cast(index) };\n> +\t\top.data = { RPiBufferMask::STATS | static_cast(index) };\n> \t\tipa_->processEvent(op);\n> \t} else {\n> \t\t/* Any other ISP output can be handed back to the application now. */\n> @@ -1293,6 +1303,11 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre\n> \t\tRequest *request = requestQueue_.front();\n> \n> \t\tif (!dropFrameCount_ && request->findBuffer(stream) == buffer) {\n> +\t\t\t/*\n> +\t\t\t * Check if this is an externally provided buffer, and if\n> +\t\t\t * so, we must stop tracking it in the pipeline handler.\n> +\t\t\t */\n> +\t\t\thandleExternalBuffer(buffer, stream);\n> \t\t\t/*\n> \t\t\t * Tag the buffer as completed, returning it to the\n> \t\t\t * application.\n> @@ -1311,6 +1326,17 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre\n> \t}\n> }\n> \n> +void RPiCameraData::handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)\n> +{\n> +\tunsigned int id = stream->getBufferId(buffer);\n> +\n> +\tif (!(id & RPiBufferMask::EXTERNAL_BUFFER))\n> +\t\treturn;\n> +\n> +\t/* Stop the Stream object from tracking the buffer. */\n> +\tstream->removeExternalBuffer(buffer);\n> +}\n> +\n> void RPiCameraData::handleState()\n> {\n> \tswitch (state_) {\n> @@ -1443,8 +1469,8 @@ void RPiCameraData::tryRunPipeline()\n> \t\t\t<< \" Embedded buffer id: \" << embeddedIndex;\n> \n> \top.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE;\n> -\top.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex,\n> -\t\t RPiIpaMask::BAYER_DATA | bayerIndex };\n> +\top.data = { RPiBufferMask::EMBEDDED_DATA | embeddedIndex,\n> +\t\t RPiBufferMask::BAYER_DATA | bayerIndex };\n> \tipa_->processEvent(op);\n> }\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> index 879e25ba..c09f14c9 100644\n> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> @@ -6,6 +6,8 @@\n> */\n> #include \"rpi_stream.h\"\n> \n> +#include \n> +\n> #include \"libcamera/internal/log.h\"\n> \n> namespace libcamera {\n> @@ -44,8 +46,13 @@ bool RPiStream::isExternal() const\n> \n> void RPiStream::setExportedBuffers(std::vector> *buffers)\n> {\n> -\tfor (auto const &buffer : *buffers)\n> -\t\tbufferMap_.emplace(id_++, buffer.get());\n> +\t/* Ensure we are using a sensible number of buffers. */\n> +\tASSERT(id_ < RPiBufferMask::ID);\n> +\n> +\tfor (auto const &buffer : *buffers) {\n> +\t\tbufferMap_.emplace(id_ & RPiBufferMask::ID, buffer.get());\n> +\t\tid_++;\n> +\t}\n> }\n> \n> const BufferMap &RPiStream::getBuffers() const\n> @@ -68,6 +75,25 @@ int RPiStream::getBufferId(FrameBuffer *buffer) const\n> \treturn it->first;\n> }\n> \n> +void RPiStream::setExternalBuffer(FrameBuffer *buffer)\n> +{\n> +\t/* Ensure we are using a sensible number of buffers. */\n> +\tASSERT(id_ < RPiBufferMask::ID);\n> +\n> +\tbufferMap_.emplace(RPiBufferMask::EXTERNAL_BUFFER | (id_ & RPiBufferMask::ID),\n> +\t\t\t buffer);\n> +\tid_++;\n> +}\n> +\n> +void RPiStream::removeExternalBuffer(FrameBuffer *buffer)\n> +{\n> +\tint id = getBufferId(buffer);\n> +\n> +\t/* Ensure we have this buffer in the stream, and it is marked external. */\n> +\tASSERT(id != -1 && (id & RPiBufferMask::EXTERNAL_BUFFER));\n> +\tbufferMap_.erase(id);\n> +}\n> +\n> int RPiStream::prepareBuffers(unsigned int count)\n> {\n> \tint ret;\n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> index ed517c22..8b23c4b2 100644\n> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> @@ -51,6 +51,9 @@ public:\n> \tconst BufferMap &getBuffers() const;\n> \tint getBufferId(FrameBuffer *buffer) const;\n> \n> +\tvoid setExternalBuffer(FrameBuffer *buffer);\n> +\tvoid removeExternalBuffer(FrameBuffer *buffer);\n> +\n> \tint prepareBuffers(unsigned int count);\n> \tint queueBuffer(FrameBuffer *buffer);\n> \tvoid returnBuffer(FrameBuffer *buffer);\n> -- \n> 2.25.1\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1238BBF01C\n\tfor ;\n\tSat, 12 Sep 2020 15:10:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8023C62DA1;\n\tSat, 12 Sep 2020 17:10:38 +0200 (CEST)","from mail-lf1-x12e.google.com (mail-lf1-x12e.google.com\n\t[IPv6:2a00:1450:4864:20::12e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2BE0062C8C\n\tfor ;\n\tSat, 12 Sep 2020 17:10:37 +0200 (CEST)","by mail-lf1-x12e.google.com with SMTP id m5so8862441lfp.7\n\tfor ;\n\tSat, 12 Sep 2020 08:10:37 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tb197sm1072346lfd.251.2020.09.12.08.10.35\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 12 Sep 2020 08:10:35 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"f7m06vou\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=Vc0+TykySlzdilO6c6/dEBzhCNb7ODwQFwT5u7+WCvE=;\n\tb=f7m06vou6Thyus7AxP+8UhroypKzQrxFxGqcQqfAEIItOoNzHH6XeYby5kXP8sIhFt\n\tZxy7CHikEDUIl0wXjMWRFBLR3p5HWVyRgXMybBJhcFd8yIN5Xf3Kt28pHZutd/e1wZyt\n\tpZ8Gd3/DwSkLhyXR2VLsyWHtMseuNtMtIPVYiSoK/vrc8wDagY7N2Bc64jqG/lKsWlc3\n\t29wLFy1wNz6KCJXYznz/p4ROQms5MwDsi2bhRAjvuG/kLwVZGFdkwvRnNlMfIJ32Bg8c\n\t6xQhBBoo7SqSNvyQyRaG1sQrmEwfCf6wS6+LX8sdODVfmKNbfyRjk60BrhxJqQPrF8vw\n\tSWrg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=Vc0+TykySlzdilO6c6/dEBzhCNb7ODwQFwT5u7+WCvE=;\n\tb=LNGENFXD8e2XmY+hKO8T3LqmvP4kWosLmRDEarzHpbOjgEDA1y7BKTyd18rdZMGhtp\n\tTgWguwpq12NxLEzmNitQcMMyBvtml5HLSC98eXBAuODUXLc54KtgPhRgJ1N9gsxEyczU\n\tDe74+O8sYntwiT6TKXjpC4mGZ6Ui3PyAK/ssw0DoBtO5DK4XftCrySZ144nSWa0o+24W\n\tsdfUx1eInCh4RA7N0qFnwmg1QoynvIFyAJdP5luB5hZDWxovnOeZ4/LB04Gn0QywX02i\n\tP52q2qcNPcU9LbVQwmkUBHRnkhuKLOuRjt5V4hysdSoSaXS5Ol+3z0e1lVrGm/ldui6L\n\tvcpA==","X-Gm-Message-State":"AOAM533DMMarMtxktYy3T1TiSKilDnePn8LeBMBHXZ4hbkZQqasChLI7\n\tLza3K4Xcm+9QbZgEj1649tAsaw==","X-Google-Smtp-Source":"ABdhPJyGEMYjQPNld6/gWCZjiOMZXxr3Yq33Xcx1pqb7c3JLNXvoolhMQj/OzYeCztV1b7hyelI3Ig==","X-Received":"by 2002:a19:c68f:: with SMTP id\n\tw137mr2112652lff.213.1599923436404; \n\tSat, 12 Sep 2020 08:10:36 -0700 (PDT)","Date":"Sat, 12 Sep 2020 17:10:35 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Naushir Patuck ","Message-ID":"<20200912151035.GD695456@oden.dyn.berto.se>","References":"<20200908074913.109244-1-naush@raspberrypi.com>\n\t<20200908074913.109244-13-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200908074913.109244-13-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v7 12/12] pipeline: ipa: raspberrypi:\n\tHandle any externally allocated FrameBuffer","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=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12524,"web_url":"https://patchwork.libcamera.org/comment/12524/","msgid":"<876808e9-66ac-f009-9d4b-d6bbabd4fd73@ideasonboard.com>","date":"2020-09-15T13:59:23","subject":"Re: [libcamera-devel] [PATCH v7 12/12] pipeline: ipa: raspberrypi:\n\tHandle any externally allocated FrameBuffer","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush,\n\nOn 08/09/2020 08:49, Naushir Patuck wrote:\n> Handle the case where a FrameBuffer that has been externally allocated\n> (i.e. not through the v4l2 video device) is passed into a Request.\n> \n> We must store the buffer pointer in the stream internal buffer list to\n> identify when used.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> include/libcamera/ipa/raspberrypi.h | 11 ++--\n> src/ipa/raspberrypi/raspberrypi.cpp | 6 +-\n> .../pipeline/raspberrypi/raspberrypi.cpp | 64 +++++++++++++------\n> .../pipeline/raspberrypi/rpi_stream.cpp | 30 ++++++++-\n> .../pipeline/raspberrypi/rpi_stream.h | 3 +\n> 5 files changed, 85 insertions(+), 29 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index 262fc6f3..dd6ebeac 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -28,11 +28,12 @@ enum RPiOperations {\n> \tRPI_IPA_EVENT_QUEUE_REQUEST,\n> };\n> \n> -enum RPiIpaMask {\n> -\tID\t\t= 0x0ffff,\n> -\tSTATS\t\t= 0x10000,\n> -\tEMBEDDED_DATA\t= 0x20000,\n> -\tBAYER_DATA\t= 0x40000\n> +enum RPiBufferMask {\n> +\tID\t\t= 0x00ffff,\n> +\tSTATS\t\t= 0x010000,\n> +\tEMBEDDED_DATA\t= 0x020000,\n> +\tBAYER_DATA\t= 0x040000,\n> +\tEXTERNAL_BUFFER\t= 0x100000,\n> };\n> \n> /* Size of the LS grid allocation. */\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 688d2efc..0555cc4e 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -352,7 +352,7 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> \n> \t\tIPAOperationData op;\n> \t\top.operation = RPI_IPA_ACTION_STATS_METADATA_COMPLETE;\n> -\t\top.data = { bufferId & RPiIpaMask::ID };\n> +\t\top.data = { bufferId & RPiBufferMask::ID };\n> \t\top.controls = { libcameraMetadata_ };\n> \t\tqueueFrameAction.emit(0, op);\n> \t\tbreak;\n> @@ -373,7 +373,7 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> \t\t/* Ready to push the input buffer into the ISP. */\n> \t\tIPAOperationData op;\n> \t\top.operation = RPI_IPA_ACTION_RUN_ISP;\n> -\t\top.data = { bayerbufferId & RPiIpaMask::ID };\n> +\t\top.data = { bayerbufferId & RPiBufferMask::ID };\n> \t\tqueueFrameAction.emit(0, op);\n> \t\tbreak;\n> \t}\n> @@ -700,7 +700,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> {\n> \tIPAOperationData op;\n> \top.operation = RPI_IPA_ACTION_EMBEDDED_COMPLETE;\n> -\top.data = { bufferId & RPiIpaMask::ID };\n> +\top.data = { bufferId & RPiBufferMask::ID };\n> \tqueueFrameAction.emit(0, op);\n> }\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 5621f182..f5e0d1cd 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -151,6 +151,7 @@ public:\n> \n> \tvoid clearIncompleteRequests();\n> \tvoid handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);\n> +\tvoid handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);\n> \tvoid handleState();\n> \n> \tCameraSensor *sensor_;\n> @@ -725,23 +726,32 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> \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> -\t\t\tFrameBuffer *buffer = request->findBuffer(stream);\n> +\t\tif (!stream->isExternal())\n> +\t\t\tcontinue;\n> +\n> +\t\tFrameBuffer *buffer = request->findBuffer(stream);\n> +\t\tif (buffer && stream->getBufferId(buffer) == -1) {\n> \t\t\t/*\n> -\t\t\t * If no buffer is provided by the request for this stream, we\n> -\t\t\t * queue a nullptr to the stream to signify that it must use an\n> -\t\t\t * internally allocated buffer for this capture request. This\n> -\t\t\t * buffer will not be given back to the application, but is used\n> -\t\t\t * to support the internal pipeline flow.\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 * This buffer is not recognised, so it must have been allocated\n> +\t\t\t * outside the v4l2 device. Store it in the stream buffer list\n> +\t\t\t * so we can track it.\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\tstream->setExternalBuffer(buffer);\n> \t\t}\n> +\t\t/*\n> +\t\t * If no buffer is provided by the request for this stream, we\n> +\t\t * queue a nullptr to the stream to signify that it must use an\n> +\t\t * internally allocated buffer for this capture request. This\n> +\t\t * buffer will not be given back to the application, but is used\n> +\t\t * to support the internal pipeline flow.\n> +\t\t *\n> +\t\t * The below queueBuffer() call will do nothing if there are not\n> +\t\t * enough internal buffers allocated, but this will be handled by\n> +\t\t * queuing the request for buffers in the RPiStream object.\n> +\t\t */\n> +\t\tint ret = stream->queueBuffer(buffer);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> \t}\n> \n> \t/* Push the request to the back of the queue. */\n> @@ -915,8 +925,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> \t * Pass the stats and embedded data buffers to the IPA. No other\n> \t * buffers need to be passed.\n> \t */\n> -\tmapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPiIpaMask::STATS);\n> -\tmapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), RPiIpaMask::EMBEDDED_DATA);\n> +\tmapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPiBufferMask::STATS);\n> +\tmapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), RPiBufferMask::EMBEDDED_DATA);\n> \n> \treturn 0;\n> }\n> @@ -1219,7 +1229,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\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 | static_cast(index) };\n> +\t\top.data = { RPiBufferMask::STATS | static_cast(index) };\n> \t\tipa_->processEvent(op);\n> \t} else {\n> \t\t/* Any other ISP output can be handed back to the application now. */\n> @@ -1293,6 +1303,11 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre\n> \t\tRequest *request = requestQueue_.front();\n> \n> \t\tif (!dropFrameCount_ && request->findBuffer(stream) == buffer) {\n> +\t\t\t/*\n> +\t\t\t * Check if this is an externally provided buffer, and if\n> +\t\t\t * so, we must stop tracking it in the pipeline handler.\n> +\t\t\t */\n> +\t\t\thandleExternalBuffer(buffer, stream);\n> \t\t\t/*\n> \t\t\t * Tag the buffer as completed, returning it to the\n> \t\t\t * application.\n> @@ -1311,6 +1326,17 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre\n> \t}\n> }\n> \n> +void RPiCameraData::handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)\n> +{\n> +\tunsigned int id = stream->getBufferId(buffer);\n> +\n> +\tif (!(id & RPiBufferMask::EXTERNAL_BUFFER))\n> +\t\treturn;\n> +\n> +\t/* Stop the Stream object from tracking the buffer. */\n> +\tstream->removeExternalBuffer(buffer);\n> +}\n> +\n> void RPiCameraData::handleState()\n> {\n> \tswitch (state_) {\n> @@ -1443,8 +1469,8 @@ void RPiCameraData::tryRunPipeline()\n> \t\t\t<< \" Embedded buffer id: \" << embeddedIndex;\n> \n> \top.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE;\n> -\top.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex,\n> -\t\t RPiIpaMask::BAYER_DATA | bayerIndex };\n> +\top.data = { RPiBufferMask::EMBEDDED_DATA | embeddedIndex,\n> +\t\t RPiBufferMask::BAYER_DATA | bayerIndex };\n> \tipa_->processEvent(op);\n> }\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> index 879e25ba..c09f14c9 100644\n> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> @@ -6,6 +6,8 @@\n> */\n> #include \"rpi_stream.h\"\n> \n> +#include \n> +\n> #include \"libcamera/internal/log.h\"\n> \n> namespace libcamera {\n> @@ -44,8 +46,13 @@ bool RPiStream::isExternal() const\n> \n> void RPiStream::setExportedBuffers(std::vector> *buffers)\n> {\n> -\tfor (auto const &buffer : *buffers)\n> -\t\tbufferMap_.emplace(id_++, buffer.get());\n> +\t/* Ensure we are using a sensible number of buffers. */\n> +\tASSERT(id_ < RPiBufferMask::ID);\n> +\n> +\tfor (auto const &buffer : *buffers) {\n> +\t\tbufferMap_.emplace(id_ & RPiBufferMask::ID, buffer.get());\n> +\t\tid_++;\n> +\t}\n\nI half wonder if that assert shouldn't come 'after' the updates to the\nbufferMap to catch if the id_ has overflowed after increase.\n\nBut maybe this is a tiny corner case and not expected anyway.\n\n\n> }\n> \n> const BufferMap &RPiStream::getBuffers() const\n> @@ -68,6 +75,25 @@ int RPiStream::getBufferId(FrameBuffer *buffer) const\n> \treturn it->first;\n> }\n> \n> +void RPiStream::setExternalBuffer(FrameBuffer *buffer)\n> +{\n> +\t/* Ensure we are using a sensible number of buffers. */\n> +\tASSERT(id_ < RPiBufferMask::ID);\n> +\n> +\tbufferMap_.emplace(RPiBufferMask::EXTERNAL_BUFFER | (id_ & RPiBufferMask::ID),\n> +\t\t\t buffer);\n> +\tid_++;\n\nOh, ... is there no reduction of the id_?\n\nWhat happens if an application continually provides an external buffer\n... won't that constantly increase this id_ with every frame?\n\nAre the ID's re-used? or is it just that the stream doesn't generate a\nnew ID for each usage of an external buffer (i.e. it really would have\nto be more than 65535 entirely different buffers...)\n\nI guess this can be easily validated by setting RPiBufferMask::ID to\n0xFF or even 0xF to see if it hits any assertions quickly\n\nAs long as this doesn't leak and cause an assertion/overflow after\n0xFFFF/RPiBufferMask::ID requests are queued ...\n\nReviewed-by: Kieran Bingham \n\n> +}\n> +\n> +void RPiStream::removeExternalBuffer(FrameBuffer *buffer)\n> +{\n> +\tint id = getBufferId(buffer);\n> +\n> +\t/* Ensure we have this buffer in the stream, and it is marked external. */\n> +\tASSERT(id != -1 && (id & RPiBufferMask::EXTERNAL_BUFFER));\n> +\tbufferMap_.erase(id);\n> +}\n> +\n> int RPiStream::prepareBuffers(unsigned int count)\n> {\n> \tint ret;\n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> index ed517c22..8b23c4b2 100644\n> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> @@ -51,6 +51,9 @@ public:\n> \tconst BufferMap &getBuffers() const;\n> \tint getBufferId(FrameBuffer *buffer) const;\n> \n> +\tvoid setExternalBuffer(FrameBuffer *buffer);\n> +\tvoid removeExternalBuffer(FrameBuffer *buffer);\n> +\n> \tint prepareBuffers(unsigned int count);\n> \tint queueBuffer(FrameBuffer *buffer);\n> \tvoid returnBuffer(FrameBuffer *buffer);\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 79539C3B5B\n\tfor ;\n\tTue, 15 Sep 2020 13:59:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D32F162E0B;\n\tTue, 15 Sep 2020 15:59:27 +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 3247F6036C\n\tfor ;\n\tTue, 15 Sep 2020 15:59:26 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9D38C275;\n\tTue, 15 Sep 2020 15:59:25 +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=\"OuepG5Ls\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600178365;\n\tbh=FCokMY+CtUlCdAIzL5OMxcX5J1C2M220t4UpQYvdB7A=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=OuepG5LseeLVBIkdXV5RN5OKjr/VRug7t0JamdfQpp2Dmph69VDFnw58DnIkgHJCO\n\td/peyZ5gqVh0e28NyoqTG+FBufxnxo9Jb7JdgOkax0XnYRJBTWZ72ZbaGrl2O+ujDq\n\t8HXa8krnQEYyPkqzOqFWmTDYhMLtc9O6doToTAu8=","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20200908074913.109244-1-naush@raspberrypi.com>\n\t<20200908074913.109244-13-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":"<876808e9-66ac-f009-9d4b-d6bbabd4fd73@ideasonboard.com>","Date":"Tue, 15 Sep 2020 14:59:23 +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":"<20200908074913.109244-13-naush@raspberrypi.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v7 12/12] pipeline: ipa: raspberrypi:\n\tHandle any externally allocated FrameBuffer","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12525,"web_url":"https://patchwork.libcamera.org/comment/12525/","msgid":"<20200915145534.GA1850958@oden.dyn.berto.se>","date":"2020-09-15T14:55:34","subject":"Re: [libcamera-devel] [PATCH v7 12/12] pipeline: ipa: raspberrypi:\n\tHandle any externally allocated FrameBuffer","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Naush, Kieran,\n\nOn 2020-09-15 14:59:23 +0100, Kieran Bingham wrote:\n> Hi Naush,\n> \n> On 08/09/2020 08:49, Naushir Patuck wrote:\n> > Handle the case where a FrameBuffer that has been externally allocated\n> > (i.e. not through the v4l2 video device) is passed into a Request.\n> > \n> > We must store the buffer pointer in the stream internal buffer list to\n> > identify when used.\n> > \n> > Signed-off-by: Naushir Patuck \n> > ---\n> > include/libcamera/ipa/raspberrypi.h | 11 ++--\n> > src/ipa/raspberrypi/raspberrypi.cpp | 6 +-\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 64 +++++++++++++------\n> > .../pipeline/raspberrypi/rpi_stream.cpp | 30 ++++++++-\n> > .../pipeline/raspberrypi/rpi_stream.h | 3 +\n> > 5 files changed, 85 insertions(+), 29 deletions(-)\n> > \n> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > index 262fc6f3..dd6ebeac 100644\n> > --- a/include/libcamera/ipa/raspberrypi.h\n> > +++ b/include/libcamera/ipa/raspberrypi.h\n> > @@ -28,11 +28,12 @@ enum RPiOperations {\n> > \tRPI_IPA_EVENT_QUEUE_REQUEST,\n> > };\n> > \n> > -enum RPiIpaMask {\n> > -\tID\t\t= 0x0ffff,\n> > -\tSTATS\t\t= 0x10000,\n> > -\tEMBEDDED_DATA\t= 0x20000,\n> > -\tBAYER_DATA\t= 0x40000\n> > +enum RPiBufferMask {\n> > +\tID\t\t= 0x00ffff,\n> > +\tSTATS\t\t= 0x010000,\n> > +\tEMBEDDED_DATA\t= 0x020000,\n> > +\tBAYER_DATA\t= 0x040000,\n> > +\tEXTERNAL_BUFFER\t= 0x100000,\n> > };\n> > \n> > /* Size of the LS grid allocation. */\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 688d2efc..0555cc4e 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -352,7 +352,7 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> > \n> > \t\tIPAOperationData op;\n> > \t\top.operation = RPI_IPA_ACTION_STATS_METADATA_COMPLETE;\n> > -\t\top.data = { bufferId & RPiIpaMask::ID };\n> > +\t\top.data = { bufferId & RPiBufferMask::ID };\n> > \t\top.controls = { libcameraMetadata_ };\n> > \t\tqueueFrameAction.emit(0, op);\n> > \t\tbreak;\n> > @@ -373,7 +373,7 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> > \t\t/* Ready to push the input buffer into the ISP. */\n> > \t\tIPAOperationData op;\n> > \t\top.operation = RPI_IPA_ACTION_RUN_ISP;\n> > -\t\top.data = { bayerbufferId & RPiIpaMask::ID };\n> > +\t\top.data = { bayerbufferId & RPiBufferMask::ID };\n> > \t\tqueueFrameAction.emit(0, op);\n> > \t\tbreak;\n> > \t}\n> > @@ -700,7 +700,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> > {\n> > \tIPAOperationData op;\n> > \top.operation = RPI_IPA_ACTION_EMBEDDED_COMPLETE;\n> > -\top.data = { bufferId & RPiIpaMask::ID };\n> > +\top.data = { bufferId & RPiBufferMask::ID };\n> > \tqueueFrameAction.emit(0, op);\n> > }\n> > \n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 5621f182..f5e0d1cd 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -151,6 +151,7 @@ public:\n> > \n> > \tvoid clearIncompleteRequests();\n> > \tvoid handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);\n> > +\tvoid handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);\n> > \tvoid handleState();\n> > \n> > \tCameraSensor *sensor_;\n> > @@ -725,23 +726,32 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> > \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> > -\t\t\tFrameBuffer *buffer = request->findBuffer(stream);\n> > +\t\tif (!stream->isExternal())\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tFrameBuffer *buffer = request->findBuffer(stream);\n> > +\t\tif (buffer && stream->getBufferId(buffer) == -1) {\n> > \t\t\t/*\n> > -\t\t\t * If no buffer is provided by the request for this stream, we\n> > -\t\t\t * queue a nullptr to the stream to signify that it must use an\n> > -\t\t\t * internally allocated buffer for this capture request. This\n> > -\t\t\t * buffer will not be given back to the application, but is used\n> > -\t\t\t * to support the internal pipeline flow.\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 * This buffer is not recognised, so it must have been allocated\n> > +\t\t\t * outside the v4l2 device. Store it in the stream buffer list\n> > +\t\t\t * so we can track it.\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\tstream->setExternalBuffer(buffer);\n> > \t\t}\n> > +\t\t/*\n> > +\t\t * If no buffer is provided by the request for this stream, we\n> > +\t\t * queue a nullptr to the stream to signify that it must use an\n> > +\t\t * internally allocated buffer for this capture request. This\n> > +\t\t * buffer will not be given back to the application, but is used\n> > +\t\t * to support the internal pipeline flow.\n> > +\t\t *\n> > +\t\t * The below queueBuffer() call will do nothing if there are not\n> > +\t\t * enough internal buffers allocated, but this will be handled by\n> > +\t\t * queuing the request for buffers in the RPiStream object.\n> > +\t\t */\n> > +\t\tint ret = stream->queueBuffer(buffer);\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n> > \t}\n> > \n> > \t/* Push the request to the back of the queue. */\n> > @@ -915,8 +925,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > \t * Pass the stats and embedded data buffers to the IPA. No other\n> > \t * buffers need to be passed.\n> > \t */\n> > -\tmapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPiIpaMask::STATS);\n> > -\tmapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), RPiIpaMask::EMBEDDED_DATA);\n> > +\tmapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPiBufferMask::STATS);\n> > +\tmapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), RPiBufferMask::EMBEDDED_DATA);\n> > \n> > \treturn 0;\n> > }\n> > @@ -1219,7 +1229,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\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 | static_cast(index) };\n> > +\t\top.data = { RPiBufferMask::STATS | static_cast(index) };\n> > \t\tipa_->processEvent(op);\n> > \t} else {\n> > \t\t/* Any other ISP output can be handed back to the application now. */\n> > @@ -1293,6 +1303,11 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre\n> > \t\tRequest *request = requestQueue_.front();\n> > \n> > \t\tif (!dropFrameCount_ && request->findBuffer(stream) == buffer) {\n> > +\t\t\t/*\n> > +\t\t\t * Check if this is an externally provided buffer, and if\n> > +\t\t\t * so, we must stop tracking it in the pipeline handler.\n> > +\t\t\t */\n> > +\t\t\thandleExternalBuffer(buffer, stream);\n> > \t\t\t/*\n> > \t\t\t * Tag the buffer as completed, returning it to the\n> > \t\t\t * application.\n> > @@ -1311,6 +1326,17 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre\n> > \t}\n> > }\n> > \n> > +void RPiCameraData::handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)\n> > +{\n> > +\tunsigned int id = stream->getBufferId(buffer);\n> > +\n> > +\tif (!(id & RPiBufferMask::EXTERNAL_BUFFER))\n> > +\t\treturn;\n> > +\n> > +\t/* Stop the Stream object from tracking the buffer. */\n> > +\tstream->removeExternalBuffer(buffer);\n> > +}\n> > +\n> > void RPiCameraData::handleState()\n> > {\n> > \tswitch (state_) {\n> > @@ -1443,8 +1469,8 @@ void RPiCameraData::tryRunPipeline()\n> > \t\t\t<< \" Embedded buffer id: \" << embeddedIndex;\n> > \n> > \top.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE;\n> > -\top.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex,\n> > -\t\t RPiIpaMask::BAYER_DATA | bayerIndex };\n> > +\top.data = { RPiBufferMask::EMBEDDED_DATA | embeddedIndex,\n> > +\t\t RPiBufferMask::BAYER_DATA | bayerIndex };\n> > \tipa_->processEvent(op);\n> > }\n> > \n> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > index 879e25ba..c09f14c9 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > @@ -6,6 +6,8 @@\n> > */\n> > #include \"rpi_stream.h\"\n> > \n> > +#include \n> > +\n> > #include \"libcamera/internal/log.h\"\n> > \n> > namespace libcamera {\n> > @@ -44,8 +46,13 @@ bool RPiStream::isExternal() const\n> > \n> > void RPiStream::setExportedBuffers(std::vector> *buffers)\n> > {\n> > -\tfor (auto const &buffer : *buffers)\n> > -\t\tbufferMap_.emplace(id_++, buffer.get());\n> > +\t/* Ensure we are using a sensible number of buffers. */\n> > +\tASSERT(id_ < RPiBufferMask::ID);\n> > +\n> > +\tfor (auto const &buffer : *buffers) {\n> > +\t\tbufferMap_.emplace(id_ & RPiBufferMask::ID, buffer.get());\n> > +\t\tid_++;\n> > +\t}\n> \n> I half wonder if that assert shouldn't come 'after' the updates to the\n> bufferMap to catch if the id_ has overflowed after increase.\n> \n> But maybe this is a tiny corner case and not expected anyway.\n> \n> \n> > }\n> > \n> > const BufferMap &RPiStream::getBuffers() const\n> > @@ -68,6 +75,25 @@ int RPiStream::getBufferId(FrameBuffer *buffer) const\n> > \treturn it->first;\n> > }\n> > \n> > +void RPiStream::setExternalBuffer(FrameBuffer *buffer)\n> > +{\n> > +\t/* Ensure we are using a sensible number of buffers. */\n> > +\tASSERT(id_ < RPiBufferMask::ID);\n> > +\n> > +\tbufferMap_.emplace(RPiBufferMask::EXTERNAL_BUFFER | (id_ & RPiBufferMask::ID),\n> > +\t\t\t buffer);\n> > +\tid_++;\n> \n> Oh, ... is there no reduction of the id_?\n> \n> What happens if an application continually provides an external buffer\n> ... won't that constantly increase this id_ with every frame?\n> \n> Are the ID's re-used? or is it just that the stream doesn't generate a\n> new ID for each usage of an external buffer (i.e. it really would have\n> to be more than 65535 entirely different buffers...)\n> \n> I guess this can be easily validated by setting RPiBufferMask::ID to\n> 0xFF or even 0xF to see if it hits any assertions quickly\n> \n> As long as this doesn't leak and cause an assertion/overflow after\n> 0xFFFF/RPiBufferMask::ID requests are queued ...\n\nThis is a very good observation that I missed in my review. Good thing \nyou looked at it as I was about to merge this series.\n\nNaush would it be possible to rework this logic slightly to makesure the \nid_ used can rollover while at the same time is not overwriting an \nexisting mapping? With this solved I think we are ready to merge this \nseries \\o/.\n\n> \n> Reviewed-by: Kieran Bingham \n> \n> > +}\n> > +\n> > +void RPiStream::removeExternalBuffer(FrameBuffer *buffer)\n> > +{\n> > +\tint id = getBufferId(buffer);\n> > +\n> > +\t/* Ensure we have this buffer in the stream, and it is marked external. */\n> > +\tASSERT(id != -1 && (id & RPiBufferMask::EXTERNAL_BUFFER));\n> > +\tbufferMap_.erase(id);\n> > +}\n> > +\n> > int RPiStream::prepareBuffers(unsigned int count)\n> > {\n> > \tint ret;\n> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > index ed517c22..8b23c4b2 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > @@ -51,6 +51,9 @@ public:\n> > \tconst BufferMap &getBuffers() const;\n> > \tint getBufferId(FrameBuffer *buffer) const;\n> > \n> > +\tvoid setExternalBuffer(FrameBuffer *buffer);\n> > +\tvoid removeExternalBuffer(FrameBuffer *buffer);\n> > +\n> > \tint prepareBuffers(unsigned int count);\n> > \tint queueBuffer(FrameBuffer *buffer);\n> > \tvoid returnBuffer(FrameBuffer *buffer);\n> > \n> \n> -- \n> Regards\n> --\n> Kieran\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 50BC5BF01C\n\tfor ;\n\tTue, 15 Sep 2020 14:55:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A34DF62E23;\n\tTue, 15 Sep 2020 16:55:38 +0200 (CEST)","from mail-lj1-x234.google.com (mail-lj1-x234.google.com\n\t[IPv6:2a00:1450:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2D10F62D27\n\tfor ;\n\tTue, 15 Sep 2020 16:55:37 +0200 (CEST)","by mail-lj1-x234.google.com with SMTP id k25so3082397ljg.9\n\tfor ;\n\tTue, 15 Sep 2020 07:55:37 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tx25sm1096601lfe.284.2020.09.15.07.55.34\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 15 Sep 2020 07:55:35 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"tdczJvoO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=jPaCElR8o4Dp79d9hA4z2qy2b8FABot6IfZfUSTpF6A=;\n\tb=tdczJvoOStD8ac4XoX7ZCgYuHxnug3t1PVFuRZAofRwHwdD3GMPbHriwufW03PeOGf\n\tYxkgxUoaPbRn7ASDUTvloHhYGInDRovyJXdnI8wnNkzNk0r6cHygkQYqxW31Qc37ZV/+\n\tjkrcQeu3vwbdNWtmNnCZtaL5XOeCv58h0cpi2hVcPX7df6N+bHLxrV0FLtSW4S5sISaU\n\t7swTYdfA4GL7LIWUii5SHW3yK/gqUlxJEh+NZ6dou93p0eiPu58HvvvCR8XZ878k+DTe\n\tpBwQLAvi1dauTl0z7Q17p7+yUlZpC/9vk1yafrfHq7zN70Iwq0ZiTqdWfNVbj+LSyo4J\n\t2O4A==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=jPaCElR8o4Dp79d9hA4z2qy2b8FABot6IfZfUSTpF6A=;\n\tb=Sjm2ZfPOIvhMk8+gl89sOeqZ1RArBQSoWOPKOPuSPNFN41m5JJx9fcZRGEs/kh4TFB\n\tT1bmJIYX5YJ1xOLxckhuajoSfyHAvaGxKSiYsWmmShdmcf+h2FwceGiGHWPhWHM0Lfrm\n\t8W7BW60gWrwL406koZY+Lfmjy0u5oZg1zkhRRKrd4f+xWcfI8VjdkwNG4tPFODlg9OEy\n\tgtpkvC2Xt9dmR5teoC9J2zzc8ljRpwebRqImg2jQXEonh6aVB9wScPIsgAyAFFuUk/67\n\tcP2AaZdbc7QLMqVWGU4Zur/Rg+f0iQd44VQM+HF7zpJ6Bz13CWgl7l5ylGZ223AxMNQe\n\t/b/Q==","X-Gm-Message-State":"AOAM532wbS9yaTfED8h4sHYnLCC8GXrKvFLwjbuc/eTVJBVZbB4evckE\n\tNtABYeKXdwrlNY33oC1e0gnaQw==","X-Google-Smtp-Source":"ABdhPJy9hw1XqZs7w2fY9nAki+r5wpVgxuOSxllIBkKl9C2Ahe2IGqTLGW2Kr8oputzSGFY1XBYgFA==","X-Received":"by 2002:a2e:8907:: with SMTP id d7mr6886511lji.206.1600181735918;\n\tTue, 15 Sep 2020 07:55:35 -0700 (PDT)","Date":"Tue, 15 Sep 2020 16:55:34 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Kieran Bingham ","Message-ID":"<20200915145534.GA1850958@oden.dyn.berto.se>","References":"<20200908074913.109244-1-naush@raspberrypi.com>\n\t<20200908074913.109244-13-naush@raspberrypi.com>\n\t<876808e9-66ac-f009-9d4b-d6bbabd4fd73@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<876808e9-66ac-f009-9d4b-d6bbabd4fd73@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v7 12/12] pipeline: ipa: raspberrypi:\n\tHandle any externally allocated FrameBuffer","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=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12526,"web_url":"https://patchwork.libcamera.org/comment/12526/","msgid":"","date":"2020-09-15T15:18:07","subject":"Re: [libcamera-devel] [PATCH v7 12/12] pipeline: ipa: raspberrypi:\n\tHandle any externally allocated FrameBuffer","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Niklas and Kieran,\n\nThank you both for the reviews.\n\nOn Tue, 15 Sep 2020 at 15:55, Niklas Söderlund\n wrote:\n>\n> Hi Naush, Kieran,\n>\n> On 2020-09-15 14:59:23 +0100, Kieran Bingham wrote:\n> > Hi Naush,\n> >\n> > On 08/09/2020 08:49, Naushir Patuck wrote:\n> > > Handle the case where a FrameBuffer that has been externally allocated\n> > > (i.e. not through the v4l2 video device) is passed into a Request.\n> > >\n> > > We must store the buffer pointer in the stream internal buffer list to\n> > > identify when used.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > include/libcamera/ipa/raspberrypi.h | 11 ++--\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 6 +-\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 64 +++++++++++++------\n> > > .../pipeline/raspberrypi/rpi_stream.cpp | 30 ++++++++-\n> > > .../pipeline/raspberrypi/rpi_stream.h | 3 +\n> > > 5 files changed, 85 insertions(+), 29 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > > index 262fc6f3..dd6ebeac 100644\n> > > --- a/include/libcamera/ipa/raspberrypi.h\n> > > +++ b/include/libcamera/ipa/raspberrypi.h\n> > > @@ -28,11 +28,12 @@ enum RPiOperations {\n> > > RPI_IPA_EVENT_QUEUE_REQUEST,\n> > > };\n> > >\n> > > -enum RPiIpaMask {\n> > > - ID = 0x0ffff,\n> > > - STATS = 0x10000,\n> > > - EMBEDDED_DATA = 0x20000,\n> > > - BAYER_DATA = 0x40000\n> > > +enum RPiBufferMask {\n> > > + ID = 0x00ffff,\n> > > + STATS = 0x010000,\n> > > + EMBEDDED_DATA = 0x020000,\n> > > + BAYER_DATA = 0x040000,\n> > > + EXTERNAL_BUFFER = 0x100000,\n> > > };\n> > >\n> > > /* Size of the LS grid allocation. */\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 688d2efc..0555cc4e 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -352,7 +352,7 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> > >\n> > > IPAOperationData op;\n> > > op.operation = RPI_IPA_ACTION_STATS_METADATA_COMPLETE;\n> > > - op.data = { bufferId & RPiIpaMask::ID };\n> > > + op.data = { bufferId & RPiBufferMask::ID };\n> > > op.controls = { libcameraMetadata_ };\n> > > queueFrameAction.emit(0, op);\n> > > break;\n> > > @@ -373,7 +373,7 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> > > /* Ready to push the input buffer into the ISP. */\n> > > IPAOperationData op;\n> > > op.operation = RPI_IPA_ACTION_RUN_ISP;\n> > > - op.data = { bayerbufferId & RPiIpaMask::ID };\n> > > + op.data = { bayerbufferId & RPiBufferMask::ID };\n> > > queueFrameAction.emit(0, op);\n> > > break;\n> > > }\n> > > @@ -700,7 +700,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> > > {\n> > > IPAOperationData op;\n> > > op.operation = RPI_IPA_ACTION_EMBEDDED_COMPLETE;\n> > > - op.data = { bufferId & RPiIpaMask::ID };\n> > > + op.data = { bufferId & RPiBufferMask::ID };\n> > > queueFrameAction.emit(0, op);\n> > > }\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 5621f182..f5e0d1cd 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -151,6 +151,7 @@ public:\n> > >\n> > > void clearIncompleteRequests();\n> > > void handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);\n> > > + void handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);\n> > > void handleState();\n> > >\n> > > CameraSensor *sensor_;\n> > > @@ -725,23 +726,32 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> > >\n> > > /* Push all buffers supplied in the Request to the respective streams. */\n> > > for (auto stream : data->streams_) {\n> > > - if (stream->isExternal()) {\n> > > - FrameBuffer *buffer = request->findBuffer(stream);\n> > > + if (!stream->isExternal())\n> > > + continue;\n> > > +\n> > > + FrameBuffer *buffer = request->findBuffer(stream);\n> > > + if (buffer && stream->getBufferId(buffer) == -1) {\n> > > /*\n> > > - * If no buffer is provided by the request for this stream, we\n> > > - * queue a nullptr to the stream to signify that it must use an\n> > > - * internally allocated buffer for this capture request. This\n> > > - * buffer will not be given back to the application, but is used\n> > > - * to support the internal pipeline flow.\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> > > + * This buffer is not recognised, so it must have been allocated\n> > > + * outside the v4l2 device. Store it in the stream buffer list\n> > > + * so we can track it.\n> > > */\n> > > - int ret = stream->queueBuffer(buffer);\n> > > - if (ret)\n> > > - return ret;\n> > > + stream->setExternalBuffer(buffer);\n> > > }\n> > > + /*\n> > > + * If no buffer is provided by the request for this stream, we\n> > > + * queue a nullptr to the stream to signify that it must use an\n> > > + * internally allocated buffer for this capture request. This\n> > > + * buffer will not be given back to the application, but is used\n> > > + * to support the internal pipeline flow.\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> > > /* Push the request to the back of the queue. */\n> > > @@ -915,8 +925,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > > * Pass the stats and embedded data buffers to the IPA. No other\n> > > * buffers need to be passed.\n> > > */\n> > > - mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPiIpaMask::STATS);\n> > > - mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), RPiIpaMask::EMBEDDED_DATA);\n> > > + mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPiBufferMask::STATS);\n> > > + mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), RPiBufferMask::EMBEDDED_DATA);\n> > >\n> > > return 0;\n> > > }\n> > > @@ -1219,7 +1229,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n> > > if (stream == &isp_[Isp::Stats]) {\n> > > IPAOperationData op;\n> > > op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;\n> > > - op.data = { RPiIpaMask::STATS | static_cast(index) };\n> > > + op.data = { RPiBufferMask::STATS | static_cast(index) };\n> > > ipa_->processEvent(op);\n> > > } else {\n> > > /* Any other ISP output can be handed back to the application now. */\n> > > @@ -1293,6 +1303,11 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre\n> > > Request *request = requestQueue_.front();\n> > >\n> > > if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {\n> > > + /*\n> > > + * Check if this is an externally provided buffer, and if\n> > > + * so, we must stop tracking it in the pipeline handler.\n> > > + */\n> > > + handleExternalBuffer(buffer, stream);\n> > > /*\n> > > * Tag the buffer as completed, returning it to the\n> > > * application.\n> > > @@ -1311,6 +1326,17 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre\n> > > }\n> > > }\n> > >\n> > > +void RPiCameraData::handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)\n> > > +{\n> > > + unsigned int id = stream->getBufferId(buffer);\n> > > +\n> > > + if (!(id & RPiBufferMask::EXTERNAL_BUFFER))\n> > > + return;\n> > > +\n> > > + /* Stop the Stream object from tracking the buffer. */\n> > > + stream->removeExternalBuffer(buffer);\n> > > +}\n> > > +\n> > > void RPiCameraData::handleState()\n> > > {\n> > > switch (state_) {\n> > > @@ -1443,8 +1469,8 @@ void RPiCameraData::tryRunPipeline()\n> > > << \" Embedded buffer id: \" << embeddedIndex;\n> > >\n> > > op.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE;\n> > > - op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex,\n> > > - RPiIpaMask::BAYER_DATA | bayerIndex };\n> > > + op.data = { RPiBufferMask::EMBEDDED_DATA | embeddedIndex,\n> > > + RPiBufferMask::BAYER_DATA | bayerIndex };\n> > > ipa_->processEvent(op);\n> > > }\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > index 879e25ba..c09f14c9 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > @@ -6,6 +6,8 @@\n> > > */\n> > > #include \"rpi_stream.h\"\n> > >\n> > > +#include \n> > > +\n> > > #include \"libcamera/internal/log.h\"\n> > >\n> > > namespace libcamera {\n> > > @@ -44,8 +46,13 @@ bool RPiStream::isExternal() const\n> > >\n> > > void RPiStream::setExportedBuffers(std::vector> *buffers)\n> > > {\n> > > - for (auto const &buffer : *buffers)\n> > > - bufferMap_.emplace(id_++, buffer.get());\n> > > + /* Ensure we are using a sensible number of buffers. */\n> > > + ASSERT(id_ < RPiBufferMask::ID);\n> > > +\n> > > + for (auto const &buffer : *buffers) {\n> > > + bufferMap_.emplace(id_ & RPiBufferMask::ID, buffer.get());\n> > > + id_++;\n> > > + }\n> >\n> > I half wonder if that assert shouldn't come 'after' the updates to the\n> > bufferMap to catch if the id_ has overflowed after increase.\n> >\n> > But maybe this is a tiny corner case and not expected anyway.\n> >\n> >\n> > > }\n> > >\n> > > const BufferMap &RPiStream::getBuffers() const\n> > > @@ -68,6 +75,25 @@ int RPiStream::getBufferId(FrameBuffer *buffer) const\n> > > return it->first;\n> > > }\n> > >\n> > > +void RPiStream::setExternalBuffer(FrameBuffer *buffer)\n> > > +{\n> > > + /* Ensure we are using a sensible number of buffers. */\n> > > + ASSERT(id_ < RPiBufferMask::ID);\n> > > +\n> > > + bufferMap_.emplace(RPiBufferMask::EXTERNAL_BUFFER | (id_ & RPiBufferMask::ID),\n> > > + buffer);\n> > > + id_++;\n> >\n> > Oh, ... is there no reduction of the id_?\n> >\n> > What happens if an application continually provides an external buffer\n> > ... won't that constantly increase this id_ with every frame?\n> >\n> > Are the ID's re-used? or is it just that the stream doesn't generate a\n> > new ID for each usage of an external buffer (i.e. it really would have\n> > to be more than 65535 entirely different buffers...)\n> >\n> > I guess this can be easily validated by setting RPiBufferMask::ID to\n> > 0xFF or even 0xF to see if it hits any assertions quickly\n> >\n> > As long as this doesn't leak and cause an assertion/overflow after\n> > 0xFFFF/RPiBufferMask::ID requests are queued ...\n>\n> This is a very good observation that I missed in my review. Good thing\n> you looked at it as I was about to merge this series.\n>\n> Naush would it be possible to rework this logic slightly to makesure the\n> id_ used can rollover while at the same time is not overwriting an\n> existing mapping? With this solved I think we are ready to merge this\n> series \\o/.\n\nCurrently we will hit an assertion if id goes over 0xffff as it is\nnever decremented. If this is going to be a problem (and it sounds\nlike it will be for android that only ever uses external buffers?), I\nwill think about how to rework this to allow for reusing a previously\nreturned id.\n\nRegards,\nNaush\n\n>\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> > > +}\n> > > +\n> > > +void RPiStream::removeExternalBuffer(FrameBuffer *buffer)\n> > > +{\n> > > + int id = getBufferId(buffer);\n> > > +\n> > > + /* Ensure we have this buffer in the stream, and it is marked external. */\n> > > + ASSERT(id != -1 && (id & RPiBufferMask::EXTERNAL_BUFFER));\n> > > + bufferMap_.erase(id);\n> > > +}\n> > > +\n> > > int RPiStream::prepareBuffers(unsigned int count)\n> > > {\n> > > int ret;\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > > index ed517c22..8b23c4b2 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > > @@ -51,6 +51,9 @@ public:\n> > > const BufferMap &getBuffers() const;\n> > > int getBufferId(FrameBuffer *buffer) const;\n> > >\n> > > + void setExternalBuffer(FrameBuffer *buffer);\n> > > + void removeExternalBuffer(FrameBuffer *buffer);\n> > > +\n> > > int prepareBuffers(unsigned int count);\n> > > int queueBuffer(FrameBuffer *buffer);\n> > > void returnBuffer(FrameBuffer *buffer);\n> > >\n> >\n> > --\n> > Regards\n> > --\n> > Kieran\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund","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 736A8C3B5B\n\tfor ;\n\tTue, 15 Sep 2020 15:18:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D90B462E27;\n\tTue, 15 Sep 2020 17:18:27 +0200 (CEST)","from mail-lf1-x12e.google.com (mail-lf1-x12e.google.com\n\t[IPv6:2a00:1450:4864:20::12e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0927462D27\n\tfor ;\n\tTue, 15 Sep 2020 17:18:26 +0200 (CEST)","by mail-lf1-x12e.google.com with SMTP id y17so3499958lfa.8\n\tfor ;\n\tTue, 15 Sep 2020 08:18:25 -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=\"ne4ljd/f\"; 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=pIy4rR9UEFJBkTIIRL20kN/tqBoobgtTXfij6JYeVDE=;\n\tb=ne4ljd/f+SW8FZ3Rk9c5ZfDmIsiHX+jW93TuLhg4EpPfS4yrxPvQtxX+fJ967skehr\n\t1mvbgMOv0kvsl80o+voXWneX69iauYqVNU53p1zv21DUTxrKGtYHGvECWCbN4hd+7/Kd\n\t4/YJlFxgbSyEAFilROY4PE6BcQKq89/qInM6HeQN5/Sd5lR/28Pk2MhaPHbjEoZ0sYmB\n\trq1rtZkhkgQvveXV3DC1BJR6nrPzz+UkHJ8xc4rdtS7O9lyZxJ6meEqTfBV+XVOr2z+M\n\tyM9BzBmIeDNbrYfMsXfLXECuzgIWVxIIt/L9A+4Olyx2ru8P8ZqEjEZGG1fUeJh7CrOc\n\tEPog==","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=pIy4rR9UEFJBkTIIRL20kN/tqBoobgtTXfij6JYeVDE=;\n\tb=lzyZOzeJbg3DSmhL389oWxNqzJSwPhEFDKSqreFX01p5A8BlZSkdjPI4MEpj5+bMut\n\tqQjGrxuu0wli6JYSYQCDwHPQ+1mtV9gWoNml5jSixPa5oX/Uq7TGtabSXZzj3l3rJZc2\n\tqAPLtc7+RojXlu/dlii7cQAYBp2/Pu8S0mdOm69p1GpOkwMVtY7uzSCZ76WVVRWH3+Re\n\tj4MDLrDXRFvVjL0UcUS5oFrFqE07dbUE/fZR/kLaCgiJuupKxgQviJP7fXjcBl8b44Cz\n\ts8L7YrpNTHBlHq8Y2+KvhSY2jZNISapZIoLd5+0/40O4fe0FRnTdpMOgMhf14VGzFo0C\n\t6Gig==","X-Gm-Message-State":"AOAM532nvhjMVIIfAjw7T1n1qwQwR3QL7rmyVz06PHv7/DRe2JCLVKwN\n\tUy2wJZXldGp3163rCWjyolUlKqAO8JSJSgWpRH62hw==","X-Google-Smtp-Source":"ABdhPJy8pLIUbT7jdvb1Au4QYDBDkHisCe00LdQlXQlvJCbpxM0om4+r8FAXlEd1D4McQSFOVD1kDa9evvvypohXeUo=","X-Received":"by 2002:a19:e03:: with SMTP id 3mr5566749lfo.488.1600183105087; \n\tTue, 15 Sep 2020 08:18:25 -0700 (PDT)","MIME-Version":"1.0","References":"<20200908074913.109244-1-naush@raspberrypi.com>\n\t<20200908074913.109244-13-naush@raspberrypi.com>\n\t<876808e9-66ac-f009-9d4b-d6bbabd4fd73@ideasonboard.com>\n\t<20200915145534.GA1850958@oden.dyn.berto.se>","In-Reply-To":"<20200915145534.GA1850958@oden.dyn.berto.se>","From":"Naushir Patuck ","Date":"Tue, 15 Sep 2020 16:18:07 +0100","Message-ID":"","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","Subject":"Re: [libcamera-devel] [PATCH v7 12/12] pipeline: ipa: raspberrypi:\n\tHandle any externally allocated FrameBuffer","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":12527,"web_url":"https://patchwork.libcamera.org/comment/12527/","msgid":"<20200915173124.GB1850958@oden.dyn.berto.se>","date":"2020-09-15T17:31:24","subject":"Re: [libcamera-devel] [PATCH v7 12/12] pipeline: ipa: raspberrypi:\n\tHandle any externally allocated FrameBuffer","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Naushir,\n\nOn 2020-09-15 16:18:07 +0100, Naushir Patuck wrote:\n> Hi Niklas and Kieran,\n> \n> Thank you both for the reviews.\n> \n> On Tue, 15 Sep 2020 at 15:55, Niklas Söderlund\n> wrote:\n> >\n> > Hi Naush, Kieran,\n> >\n> > On 2020-09-15 14:59:23 +0100, Kieran Bingham wrote:\n> > > Hi Naush,\n> > >\n> > > On 08/09/2020 08:49, Naushir Patuck wrote:\n> > > > Handle the case where a FrameBuffer that has been externally allocated\n> > > > (i.e. not through the v4l2 video device) is passed into a Request.\n> > > >\n> > > > We must store the buffer pointer in the stream internal buffer list to\n> > > > identify when used.\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > ---\n> > > > include/libcamera/ipa/raspberrypi.h | 11 ++--\n> > > > src/ipa/raspberrypi/raspberrypi.cpp | 6 +-\n> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 64 +++++++++++++------\n> > > > .../pipeline/raspberrypi/rpi_stream.cpp | 30 ++++++++-\n> > > > .../pipeline/raspberrypi/rpi_stream.h | 3 +\n> > > > 5 files changed, 85 insertions(+), 29 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > > > index 262fc6f3..dd6ebeac 100644\n> > > > --- a/include/libcamera/ipa/raspberrypi.h\n> > > > +++ b/include/libcamera/ipa/raspberrypi.h\n> > > > @@ -28,11 +28,12 @@ enum RPiOperations {\n> > > > RPI_IPA_EVENT_QUEUE_REQUEST,\n> > > > };\n> > > >\n> > > > -enum RPiIpaMask {\n> > > > - ID = 0x0ffff,\n> > > > - STATS = 0x10000,\n> > > > - EMBEDDED_DATA = 0x20000,\n> > > > - BAYER_DATA = 0x40000\n> > > > +enum RPiBufferMask {\n> > > > + ID = 0x00ffff,\n> > > > + STATS = 0x010000,\n> > > > + EMBEDDED_DATA = 0x020000,\n> > > > + BAYER_DATA = 0x040000,\n> > > > + EXTERNAL_BUFFER = 0x100000,\n> > > > };\n> > > >\n> > > > /* Size of the LS grid allocation. */\n> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > index 688d2efc..0555cc4e 100644\n> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > @@ -352,7 +352,7 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> > > >\n> > > > IPAOperationData op;\n> > > > op.operation = RPI_IPA_ACTION_STATS_METADATA_COMPLETE;\n> > > > - op.data = { bufferId & RPiIpaMask::ID };\n> > > > + op.data = { bufferId & RPiBufferMask::ID };\n> > > > op.controls = { libcameraMetadata_ };\n> > > > queueFrameAction.emit(0, op);\n> > > > break;\n> > > > @@ -373,7 +373,7 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> > > > /* Ready to push the input buffer into the ISP. */\n> > > > IPAOperationData op;\n> > > > op.operation = RPI_IPA_ACTION_RUN_ISP;\n> > > > - op.data = { bayerbufferId & RPiIpaMask::ID };\n> > > > + op.data = { bayerbufferId & RPiBufferMask::ID };\n> > > > queueFrameAction.emit(0, op);\n> > > > break;\n> > > > }\n> > > > @@ -700,7 +700,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> > > > {\n> > > > IPAOperationData op;\n> > > > op.operation = RPI_IPA_ACTION_EMBEDDED_COMPLETE;\n> > > > - op.data = { bufferId & RPiIpaMask::ID };\n> > > > + op.data = { bufferId & RPiBufferMask::ID };\n> > > > queueFrameAction.emit(0, op);\n> > > > }\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > index 5621f182..f5e0d1cd 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -151,6 +151,7 @@ public:\n> > > >\n> > > > void clearIncompleteRequests();\n> > > > void handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);\n> > > > + void handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);\n> > > > void handleState();\n> > > >\n> > > > CameraSensor *sensor_;\n> > > > @@ -725,23 +726,32 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> > > >\n> > > > /* Push all buffers supplied in the Request to the respective streams. */\n> > > > for (auto stream : data->streams_) {\n> > > > - if (stream->isExternal()) {\n> > > > - FrameBuffer *buffer = request->findBuffer(stream);\n> > > > + if (!stream->isExternal())\n> > > > + continue;\n> > > > +\n> > > > + FrameBuffer *buffer = request->findBuffer(stream);\n> > > > + if (buffer && stream->getBufferId(buffer) == -1) {\n> > > > /*\n> > > > - * If no buffer is provided by the request for this stream, we\n> > > > - * queue a nullptr to the stream to signify that it must use an\n> > > > - * internally allocated buffer for this capture request. This\n> > > > - * buffer will not be given back to the application, but is used\n> > > > - * to support the internal pipeline flow.\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> > > > + * This buffer is not recognised, so it must have been allocated\n> > > > + * outside the v4l2 device. Store it in the stream buffer list\n> > > > + * so we can track it.\n> > > > */\n> > > > - int ret = stream->queueBuffer(buffer);\n> > > > - if (ret)\n> > > > - return ret;\n> > > > + stream->setExternalBuffer(buffer);\n> > > > }\n> > > > + /*\n> > > > + * If no buffer is provided by the request for this stream, we\n> > > > + * queue a nullptr to the stream to signify that it must use an\n> > > > + * internally allocated buffer for this capture request. This\n> > > > + * buffer will not be given back to the application, but is used\n> > > > + * to support the internal pipeline flow.\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> > > > /* Push the request to the back of the queue. */\n> > > > @@ -915,8 +925,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > > > * Pass the stats and embedded data buffers to the IPA. No other\n> > > > * buffers need to be passed.\n> > > > */\n> > > > - mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPiIpaMask::STATS);\n> > > > - mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), RPiIpaMask::EMBEDDED_DATA);\n> > > > + mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPiBufferMask::STATS);\n> > > > + mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), RPiBufferMask::EMBEDDED_DATA);\n> > > >\n> > > > return 0;\n> > > > }\n> > > > @@ -1219,7 +1229,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n> > > > if (stream == &isp_[Isp::Stats]) {\n> > > > IPAOperationData op;\n> > > > op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;\n> > > > - op.data = { RPiIpaMask::STATS | static_cast(index) };\n> > > > + op.data = { RPiBufferMask::STATS | static_cast(index) };\n> > > > ipa_->processEvent(op);\n> > > > } else {\n> > > > /* Any other ISP output can be handed back to the application now. */\n> > > > @@ -1293,6 +1303,11 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre\n> > > > Request *request = requestQueue_.front();\n> > > >\n> > > > if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {\n> > > > + /*\n> > > > + * Check if this is an externally provided buffer, and if\n> > > > + * so, we must stop tracking it in the pipeline handler.\n> > > > + */\n> > > > + handleExternalBuffer(buffer, stream);\n> > > > /*\n> > > > * Tag the buffer as completed, returning it to the\n> > > > * application.\n> > > > @@ -1311,6 +1326,17 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre\n> > > > }\n> > > > }\n> > > >\n> > > > +void RPiCameraData::handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)\n> > > > +{\n> > > > + unsigned int id = stream->getBufferId(buffer);\n> > > > +\n> > > > + if (!(id & RPiBufferMask::EXTERNAL_BUFFER))\n> > > > + return;\n> > > > +\n> > > > + /* Stop the Stream object from tracking the buffer. */\n> > > > + stream->removeExternalBuffer(buffer);\n> > > > +}\n> > > > +\n> > > > void RPiCameraData::handleState()\n> > > > {\n> > > > switch (state_) {\n> > > > @@ -1443,8 +1469,8 @@ void RPiCameraData::tryRunPipeline()\n> > > > << \" Embedded buffer id: \" << embeddedIndex;\n> > > >\n> > > > op.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE;\n> > > > - op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex,\n> > > > - RPiIpaMask::BAYER_DATA | bayerIndex };\n> > > > + op.data = { RPiBufferMask::EMBEDDED_DATA | embeddedIndex,\n> > > > + RPiBufferMask::BAYER_DATA | bayerIndex };\n> > > > ipa_->processEvent(op);\n> > > > }\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > > index 879e25ba..c09f14c9 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > > @@ -6,6 +6,8 @@\n> > > > */\n> > > > #include \"rpi_stream.h\"\n> > > >\n> > > > +#include \n> > > > +\n> > > > #include \"libcamera/internal/log.h\"\n> > > >\n> > > > namespace libcamera {\n> > > > @@ -44,8 +46,13 @@ bool RPiStream::isExternal() const\n> > > >\n> > > > void RPiStream::setExportedBuffers(std::vector> *buffers)\n> > > > {\n> > > > - for (auto const &buffer : *buffers)\n> > > > - bufferMap_.emplace(id_++, buffer.get());\n> > > > + /* Ensure we are using a sensible number of buffers. */\n> > > > + ASSERT(id_ < RPiBufferMask::ID);\n> > > > +\n> > > > + for (auto const &buffer : *buffers) {\n> > > > + bufferMap_.emplace(id_ & RPiBufferMask::ID, buffer.get());\n> > > > + id_++;\n> > > > + }\n> > >\n> > > I half wonder if that assert shouldn't come 'after' the updates to the\n> > > bufferMap to catch if the id_ has overflowed after increase.\n> > >\n> > > But maybe this is a tiny corner case and not expected anyway.\n> > >\n> > >\n> > > > }\n> > > >\n> > > > const BufferMap &RPiStream::getBuffers() const\n> > > > @@ -68,6 +75,25 @@ int RPiStream::getBufferId(FrameBuffer *buffer) const\n> > > > return it->first;\n> > > > }\n> > > >\n> > > > +void RPiStream::setExternalBuffer(FrameBuffer *buffer)\n> > > > +{\n> > > > + /* Ensure we are using a sensible number of buffers. */\n> > > > + ASSERT(id_ < RPiBufferMask::ID);\n> > > > +\n> > > > + bufferMap_.emplace(RPiBufferMask::EXTERNAL_BUFFER | (id_ & RPiBufferMask::ID),\n> > > > + buffer);\n> > > > + id_++;\n> > >\n> > > Oh, ... is there no reduction of the id_?\n> > >\n> > > What happens if an application continually provides an external buffer\n> > > ... won't that constantly increase this id_ with every frame?\n> > >\n> > > Are the ID's re-used? or is it just that the stream doesn't generate a\n> > > new ID for each usage of an external buffer (i.e. it really would have\n> > > to be more than 65535 entirely different buffers...)\n> > >\n> > > I guess this can be easily validated by setting RPiBufferMask::ID to\n> > > 0xFF or even 0xF to see if it hits any assertions quickly\n> > >\n> > > As long as this doesn't leak and cause an assertion/overflow after\n> > > 0xFFFF/RPiBufferMask::ID requests are queued ...\n> >\n> > This is a very good observation that I missed in my review. Good thing\n> > you looked at it as I was about to merge this series.\n> >\n> > Naush would it be possible to rework this logic slightly to makesure the\n> > id_ used can rollover while at the same time is not overwriting an\n> > existing mapping? With this solved I think we are ready to merge this\n> > series \\o/.\n> \n> Currently we will hit an assertion if id goes over 0xffff as it is\n> never decremented. If this is going to be a problem (and it sounds\n> like it will be for android that only ever uses external buffers?), I\n> will think about how to rework this to allow for reusing a previously\n> returned id.\n\nHow about something like this,\n\n /* Find next free buffer id. */\n unsigned int id = (id_ + 1) % RPiBufferMask::ID;\n while (id != id_ && bufferMap_.find(id) != bufferMap_.end()) {\n id = (id + 1) % RPiBufferMask::ID;\n\n /* No free id could be found, this really should not happen. */\n ASSERT(id != id_);\n\n id_ = id;\n\nOf course untested ;-)\n\n> \n> Regards,\n> Naush\n> \n> >\n> > >\n> > > Reviewed-by: Kieran Bingham \n> > >\n> > > > +}\n> > > > +\n> > > > +void RPiStream::removeExternalBuffer(FrameBuffer *buffer)\n> > > > +{\n> > > > + int id = getBufferId(buffer);\n> > > > +\n> > > > + /* Ensure we have this buffer in the stream, and it is marked external. */\n> > > > + ASSERT(id != -1 && (id & RPiBufferMask::EXTERNAL_BUFFER));\n> > > > + bufferMap_.erase(id);\n> > > > +}\n> > > > +\n> > > > int RPiStream::prepareBuffers(unsigned int count)\n> > > > {\n> > > > int ret;\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > > > index ed517c22..8b23c4b2 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > > > @@ -51,6 +51,9 @@ public:\n> > > > const BufferMap &getBuffers() const;\n> > > > int getBufferId(FrameBuffer *buffer) const;\n> > > >\n> > > > + void setExternalBuffer(FrameBuffer *buffer);\n> > > > + void removeExternalBuffer(FrameBuffer *buffer);\n> > > > +\n> > > > int prepareBuffers(unsigned int count);\n> > > > int queueBuffer(FrameBuffer *buffer);\n> > > > void returnBuffer(FrameBuffer *buffer);\n> > > >\n> > >\n> > > --\n> > > Regards\n> > > --\n> > > Kieran\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund","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 3C431BF01C\n\tfor ;\n\tTue, 15 Sep 2020 17:31:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9AA9C62E13;\n\tTue, 15 Sep 2020 19:31:27 +0200 (CEST)","from mail-lf1-x12e.google.com (mail-lf1-x12e.google.com\n\t[IPv6:2a00:1450:4864:20::12e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 57865603EE\n\tfor ;\n\tTue, 15 Sep 2020 19:31:26 +0200 (CEST)","by mail-lf1-x12e.google.com with SMTP id d15so3938571lfq.11\n\tfor ;\n\tTue, 15 Sep 2020 10:31:26 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tq2sm4882802ljp.118.2020.09.15.10.31.25\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 15 Sep 2020 10:31:25 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"17D7cNo8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=nPSC+EIErOHspiBhttd25vgdaU1iKQDxFRyqdPNxyio=;\n\tb=17D7cNo8q8Zb0elbwc0qR00qNlnQNYcxDnbdcmWM0BCGSG3tKYPrU1fTq0ji4VJ2Z8\n\tk6pDzWlHqkFOls/+kVsg2KvIdtBHpaEy93nCS0lhaziW83U02q1L8rrtclBGFWtOcim6\n\tCnfz8hNvVA3HWmEm8HHbQ1oMBAp68DzLgmxwC8iw1AGPnBpTH+4dSKd+Wlfy9km5yDYI\n\tl2kB4BgmWL7OaNyLlRSiLQu58wA3AUyUyT6oLKy+xKzIq8IseB56Zu56dK/9d8XA8JCM\n\tWWQhvlRzyohMe8OV8eH/RKMGSn+y2vllmH5v4n4aPGnzpm2rDqyh7EzuDeMJkSyT+bfD\n\tbcpA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=nPSC+EIErOHspiBhttd25vgdaU1iKQDxFRyqdPNxyio=;\n\tb=YJStnCLrgBa8shkZ7oKsm3Mu1EVYEBX2NAhXam+UbdfNk4bAyWn8Zfl/NEhR+Is7z7\n\tCwocrAs9L8IETZD7ja1sqAR44GMOZLKgpyfDV1zbsSYvcCCbu2msB0RjFnZAhjidrn/p\n\t0er+vUnVdv6sX24Rc6CNDgeuGj45/wdVhK4OgvyG3h67xDqeqpc2CQDZgjVvaZvm1Hx2\n\toWcEzG8VylMDqTv4xm1BUUZHeWlOaBUeswF0IZPp8Y4SCmf8hzJeLb4AlTKaQRTURK1A\n\tcct6MaYfOKRiUSXsql5ImCkNi4ZWfSv/FSoidF+MVzj1arHlvfYuiTLyGpqTqeHHWum0\n\tBdNg==","X-Gm-Message-State":"AOAM533Y/h7kg4LuD/EDacIl9pTxYQizbNLwqKTGDc33+SV4GEg1ai3p\n\trBcLmZZKA0ibwTPDgSJJgl+9Wg==","X-Google-Smtp-Source":"ABdhPJyfBNC+8bFvOrdHU7ZbLetmwZix0/+ZhfzgYeGNSEhXF1foS11rbRG6GsZ6ySQ+m08kx7CU+A==","X-Received":"by 2002:a19:8542:: with SMTP id\n\th63mr6457515lfd.177.1600191085521; \n\tTue, 15 Sep 2020 10:31:25 -0700 (PDT)","Date":"Tue, 15 Sep 2020 19:31:24 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Naushir Patuck ","Message-ID":"<20200915173124.GB1850958@oden.dyn.berto.se>","References":"<20200908074913.109244-1-naush@raspberrypi.com>\n\t<20200908074913.109244-13-naush@raspberrypi.com>\n\t<876808e9-66ac-f009-9d4b-d6bbabd4fd73@ideasonboard.com>\n\t<20200915145534.GA1850958@oden.dyn.berto.se>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v7 12/12] pipeline: ipa: raspberrypi:\n\tHandle any externally allocated FrameBuffer","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=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12528,"web_url":"https://patchwork.libcamera.org/comment/12528/","msgid":"","date":"2020-09-15T17:40:00","subject":"Re: [libcamera-devel] [PATCH v7 12/12] pipeline: ipa: raspberrypi:\n\tHandle any externally allocated FrameBuffer","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Niklas,\n\nOn Tue, 15 Sep 2020 at 18:31, Niklas Söderlund\n wrote:\n>\n> Hi Naushir,\n>\n> On 2020-09-15 16:18:07 +0100, Naushir Patuck wrote:\n> > Hi Niklas and Kieran,\n> >\n> > Thank you both for the reviews.\n> >\n> > On Tue, 15 Sep 2020 at 15:55, Niklas Söderlund\n> > wrote:\n> > >\n> > > Hi Naush, Kieran,\n> > >\n> > > On 2020-09-15 14:59:23 +0100, Kieran Bingham wrote:\n> > > > Hi Naush,\n> > > >\n> > > > On 08/09/2020 08:49, Naushir Patuck wrote:\n> > > > > Handle the case where a FrameBuffer that has been externally allocated\n> > > > > (i.e. not through the v4l2 video device) is passed into a Request.\n> > > > >\n> > > > > We must store the buffer pointer in the stream internal buffer list to\n> > > > > identify when used.\n> > > > >\n> > > > > Signed-off-by: Naushir Patuck \n> > > > > ---\n> > > > > include/libcamera/ipa/raspberrypi.h | 11 ++--\n> > > > > src/ipa/raspberrypi/raspberrypi.cpp | 6 +-\n> > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 64 +++++++++++++------\n> > > > > .../pipeline/raspberrypi/rpi_stream.cpp | 30 ++++++++-\n> > > > > .../pipeline/raspberrypi/rpi_stream.h | 3 +\n> > > > > 5 files changed, 85 insertions(+), 29 deletions(-)\n> > > > >\n> > > > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > > > > index 262fc6f3..dd6ebeac 100644\n> > > > > --- a/include/libcamera/ipa/raspberrypi.h\n> > > > > +++ b/include/libcamera/ipa/raspberrypi.h\n> > > > > @@ -28,11 +28,12 @@ enum RPiOperations {\n> > > > > RPI_IPA_EVENT_QUEUE_REQUEST,\n> > > > > };\n> > > > >\n> > > > > -enum RPiIpaMask {\n> > > > > - ID = 0x0ffff,\n> > > > > - STATS = 0x10000,\n> > > > > - EMBEDDED_DATA = 0x20000,\n> > > > > - BAYER_DATA = 0x40000\n> > > > > +enum RPiBufferMask {\n> > > > > + ID = 0x00ffff,\n> > > > > + STATS = 0x010000,\n> > > > > + EMBEDDED_DATA = 0x020000,\n> > > > > + BAYER_DATA = 0x040000,\n> > > > > + EXTERNAL_BUFFER = 0x100000,\n> > > > > };\n> > > > >\n> > > > > /* Size of the LS grid allocation. */\n> > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > index 688d2efc..0555cc4e 100644\n> > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > @@ -352,7 +352,7 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> > > > >\n> > > > > IPAOperationData op;\n> > > > > op.operation = RPI_IPA_ACTION_STATS_METADATA_COMPLETE;\n> > > > > - op.data = { bufferId & RPiIpaMask::ID };\n> > > > > + op.data = { bufferId & RPiBufferMask::ID };\n> > > > > op.controls = { libcameraMetadata_ };\n> > > > > queueFrameAction.emit(0, op);\n> > > > > break;\n> > > > > @@ -373,7 +373,7 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> > > > > /* Ready to push the input buffer into the ISP. */\n> > > > > IPAOperationData op;\n> > > > > op.operation = RPI_IPA_ACTION_RUN_ISP;\n> > > > > - op.data = { bayerbufferId & RPiIpaMask::ID };\n> > > > > + op.data = { bayerbufferId & RPiBufferMask::ID };\n> > > > > queueFrameAction.emit(0, op);\n> > > > > break;\n> > > > > }\n> > > > > @@ -700,7 +700,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> > > > > {\n> > > > > IPAOperationData op;\n> > > > > op.operation = RPI_IPA_ACTION_EMBEDDED_COMPLETE;\n> > > > > - op.data = { bufferId & RPiIpaMask::ID };\n> > > > > + op.data = { bufferId & RPiBufferMask::ID };\n> > > > > queueFrameAction.emit(0, op);\n> > > > > }\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > index 5621f182..f5e0d1cd 100644\n> > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > @@ -151,6 +151,7 @@ public:\n> > > > >\n> > > > > void clearIncompleteRequests();\n> > > > > void handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);\n> > > > > + void handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);\n> > > > > void handleState();\n> > > > >\n> > > > > CameraSensor *sensor_;\n> > > > > @@ -725,23 +726,32 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> > > > >\n> > > > > /* Push all buffers supplied in the Request to the respective streams. */\n> > > > > for (auto stream : data->streams_) {\n> > > > > - if (stream->isExternal()) {\n> > > > > - FrameBuffer *buffer = request->findBuffer(stream);\n> > > > > + if (!stream->isExternal())\n> > > > > + continue;\n> > > > > +\n> > > > > + FrameBuffer *buffer = request->findBuffer(stream);\n> > > > > + if (buffer && stream->getBufferId(buffer) == -1) {\n> > > > > /*\n> > > > > - * If no buffer is provided by the request for this stream, we\n> > > > > - * queue a nullptr to the stream to signify that it must use an\n> > > > > - * internally allocated buffer for this capture request. This\n> > > > > - * buffer will not be given back to the application, but is used\n> > > > > - * to support the internal pipeline flow.\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> > > > > + * This buffer is not recognised, so it must have been allocated\n> > > > > + * outside the v4l2 device. Store it in the stream buffer list\n> > > > > + * so we can track it.\n> > > > > */\n> > > > > - int ret = stream->queueBuffer(buffer);\n> > > > > - if (ret)\n> > > > > - return ret;\n> > > > > + stream->setExternalBuffer(buffer);\n> > > > > }\n> > > > > + /*\n> > > > > + * If no buffer is provided by the request for this stream, we\n> > > > > + * queue a nullptr to the stream to signify that it must use an\n> > > > > + * internally allocated buffer for this capture request. This\n> > > > > + * buffer will not be given back to the application, but is used\n> > > > > + * to support the internal pipeline flow.\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> > > > > /* Push the request to the back of the queue. */\n> > > > > @@ -915,8 +925,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > > > > * Pass the stats and embedded data buffers to the IPA. No other\n> > > > > * buffers need to be passed.\n> > > > > */\n> > > > > - mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPiIpaMask::STATS);\n> > > > > - mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), RPiIpaMask::EMBEDDED_DATA);\n> > > > > + mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPiBufferMask::STATS);\n> > > > > + mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), RPiBufferMask::EMBEDDED_DATA);\n> > > > >\n> > > > > return 0;\n> > > > > }\n> > > > > @@ -1219,7 +1229,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n> > > > > if (stream == &isp_[Isp::Stats]) {\n> > > > > IPAOperationData op;\n> > > > > op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;\n> > > > > - op.data = { RPiIpaMask::STATS | static_cast(index) };\n> > > > > + op.data = { RPiBufferMask::STATS | static_cast(index) };\n> > > > > ipa_->processEvent(op);\n> > > > > } else {\n> > > > > /* Any other ISP output can be handed back to the application now. */\n> > > > > @@ -1293,6 +1303,11 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre\n> > > > > Request *request = requestQueue_.front();\n> > > > >\n> > > > > if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {\n> > > > > + /*\n> > > > > + * Check if this is an externally provided buffer, and if\n> > > > > + * so, we must stop tracking it in the pipeline handler.\n> > > > > + */\n> > > > > + handleExternalBuffer(buffer, stream);\n> > > > > /*\n> > > > > * Tag the buffer as completed, returning it to the\n> > > > > * application.\n> > > > > @@ -1311,6 +1326,17 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre\n> > > > > }\n> > > > > }\n> > > > >\n> > > > > +void RPiCameraData::handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)\n> > > > > +{\n> > > > > + unsigned int id = stream->getBufferId(buffer);\n> > > > > +\n> > > > > + if (!(id & RPiBufferMask::EXTERNAL_BUFFER))\n> > > > > + return;\n> > > > > +\n> > > > > + /* Stop the Stream object from tracking the buffer. */\n> > > > > + stream->removeExternalBuffer(buffer);\n> > > > > +}\n> > > > > +\n> > > > > void RPiCameraData::handleState()\n> > > > > {\n> > > > > switch (state_) {\n> > > > > @@ -1443,8 +1469,8 @@ void RPiCameraData::tryRunPipeline()\n> > > > > << \" Embedded buffer id: \" << embeddedIndex;\n> > > > >\n> > > > > op.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE;\n> > > > > - op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex,\n> > > > > - RPiIpaMask::BAYER_DATA | bayerIndex };\n> > > > > + op.data = { RPiBufferMask::EMBEDDED_DATA | embeddedIndex,\n> > > > > + RPiBufferMask::BAYER_DATA | bayerIndex };\n> > > > > ipa_->processEvent(op);\n> > > > > }\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > > > index 879e25ba..c09f14c9 100644\n> > > > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > > > @@ -6,6 +6,8 @@\n> > > > > */\n> > > > > #include \"rpi_stream.h\"\n> > > > >\n> > > > > +#include \n> > > > > +\n> > > > > #include \"libcamera/internal/log.h\"\n> > > > >\n> > > > > namespace libcamera {\n> > > > > @@ -44,8 +46,13 @@ bool RPiStream::isExternal() const\n> > > > >\n> > > > > void RPiStream::setExportedBuffers(std::vector> *buffers)\n> > > > > {\n> > > > > - for (auto const &buffer : *buffers)\n> > > > > - bufferMap_.emplace(id_++, buffer.get());\n> > > > > + /* Ensure we are using a sensible number of buffers. */\n> > > > > + ASSERT(id_ < RPiBufferMask::ID);\n> > > > > +\n> > > > > + for (auto const &buffer : *buffers) {\n> > > > > + bufferMap_.emplace(id_ & RPiBufferMask::ID, buffer.get());\n> > > > > + id_++;\n> > > > > + }\n> > > >\n> > > > I half wonder if that assert shouldn't come 'after' the updates to the\n> > > > bufferMap to catch if the id_ has overflowed after increase.\n> > > >\n> > > > But maybe this is a tiny corner case and not expected anyway.\n> > > >\n> > > >\n> > > > > }\n> > > > >\n> > > > > const BufferMap &RPiStream::getBuffers() const\n> > > > > @@ -68,6 +75,25 @@ int RPiStream::getBufferId(FrameBuffer *buffer) const\n> > > > > return it->first;\n> > > > > }\n> > > > >\n> > > > > +void RPiStream::setExternalBuffer(FrameBuffer *buffer)\n> > > > > +{\n> > > > > + /* Ensure we are using a sensible number of buffers. */\n> > > > > + ASSERT(id_ < RPiBufferMask::ID);\n> > > > > +\n> > > > > + bufferMap_.emplace(RPiBufferMask::EXTERNAL_BUFFER | (id_ & RPiBufferMask::ID),\n> > > > > + buffer);\n> > > > > + id_++;\n> > > >\n> > > > Oh, ... is there no reduction of the id_?\n> > > >\n> > > > What happens if an application continually provides an external buffer\n> > > > ... won't that constantly increase this id_ with every frame?\n> > > >\n> > > > Are the ID's re-used? or is it just that the stream doesn't generate a\n> > > > new ID for each usage of an external buffer (i.e. it really would have\n> > > > to be more than 65535 entirely different buffers...)\n> > > >\n> > > > I guess this can be easily validated by setting RPiBufferMask::ID to\n> > > > 0xFF or even 0xF to see if it hits any assertions quickly\n> > > >\n> > > > As long as this doesn't leak and cause an assertion/overflow after\n> > > > 0xFFFF/RPiBufferMask::ID requests are queued ...\n> > >\n> > > This is a very good observation that I missed in my review. Good thing\n> > > you looked at it as I was about to merge this series.\n> > >\n> > > Naush would it be possible to rework this logic slightly to makesure the\n> > > id_ used can rollover while at the same time is not overwriting an\n> > > existing mapping? With this solved I think we are ready to merge this\n> > > series \\o/.\n> >\n> > Currently we will hit an assertion if id goes over 0xffff as it is\n> > never decremented. If this is going to be a problem (and it sounds\n> > like it will be for android that only ever uses external buffers?), I\n> > will think about how to rework this to allow for reusing a previously\n> > returned id.\n>\n> How about something like this,\n>\n> /* Find next free buffer id. */\n> unsigned int id = (id_ + 1) % RPiBufferMask::ID;\n> while (id != id_ && bufferMap_.find(id) != bufferMap_.end()) {\n> id = (id + 1) % RPiBufferMask::ID;\n>\n> /* No free id could be found, this really should not happen. */\n> ASSERT(id != id_);\n>\n> id_ = id;\n>\n> Of course untested ;-)\n\nSomething along those lines popped into my head as well. However, I\nworry about the efficiency of map::find() on every frame, across all\nids if the map becomes dense with a large number of buffers. Instead,\nperhaps track recyclable ids in a separate queue so we can avoid the\ncostly lookup?\n\n>\n> >\n> > Regards,\n> > Naush\n> >\n> > >\n> > > >\n> > > > Reviewed-by: Kieran Bingham \n> > > >\n> > > > > +}\n> > > > > +\n> > > > > +void RPiStream::removeExternalBuffer(FrameBuffer *buffer)\n> > > > > +{\n> > > > > + int id = getBufferId(buffer);\n> > > > > +\n> > > > > + /* Ensure we have this buffer in the stream, and it is marked external. */\n> > > > > + ASSERT(id != -1 && (id & RPiBufferMask::EXTERNAL_BUFFER));\n> > > > > + bufferMap_.erase(id);\n> > > > > +}\n> > > > > +\n> > > > > int RPiStream::prepareBuffers(unsigned int count)\n> > > > > {\n> > > > > int ret;\n> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > > > > index ed517c22..8b23c4b2 100644\n> > > > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > > > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > > > > @@ -51,6 +51,9 @@ public:\n> > > > > const BufferMap &getBuffers() const;\n> > > > > int getBufferId(FrameBuffer *buffer) const;\n> > > > >\n> > > > > + void setExternalBuffer(FrameBuffer *buffer);\n> > > > > + void removeExternalBuffer(FrameBuffer *buffer);\n> > > > > +\n> > > > > int prepareBuffers(unsigned int count);\n> > > > > int queueBuffer(FrameBuffer *buffer);\n> > > > > void returnBuffer(FrameBuffer *buffer);\n> > > > >\n> > > >\n> > > > --\n> > > > Regards\n> > > > --\n> > > > Kieran\n> > > > _______________________________________________\n> > > > libcamera-devel mailing list\n> > > > libcamera-devel@lists.libcamera.org\n> > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > >\n> > > --\n> > > Regards,\n> > > Niklas Söderlund\n>\n> --\n> Regards,\n> Niklas Söderlund","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 2FFF8C3B5B\n\tfor ;\n\tTue, 15 Sep 2020 17:40:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A05DB62E1B;\n\tTue, 15 Sep 2020 19:40:20 +0200 (CEST)","from mail-lj1-x235.google.com (mail-lj1-x235.google.com\n\t[IPv6:2a00:1450:4864:20::235])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B2855603EE\n\tfor ;\n\tTue, 15 Sep 2020 19:40:18 +0200 (CEST)","by mail-lj1-x235.google.com with SMTP id c2so3544196ljj.12\n\tfor ;\n\tTue, 15 Sep 2020 10:40:18 -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=\"QHBgjNI0\"; 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=8f+67xMVVa2AYJnZOwRLL/u44JkQLpgyU2YPjLkSaBk=;\n\tb=QHBgjNI0hzXZYPeduYJo45X/zf159aDsHZ2KBgYYTiF+bPW7KfvQKr7T0XxnkA+6DD\n\t8ptpvkev1uKB4uRAvRnX5RE3napuEU427VrVsgMQKymaUYBYF29WSE7gExApOqMHrIg7\n\tJvyB1robpOZ4ngjbfuJvTlkdFeTFR2Ae9TR/Q5MsKKWVcwx2PNi6Af5itfi3smWP1fbw\n\tTgzRNwH2p6xG3t4W/9z8lx5YjlcoVYC5LDZiNc8up6UctfQ/zKbGvHvBxkwlnE+raKAg\n\tif8WhpmJ3Fhd4Jr0jUIxiUE4iCHByiBU4GSu5a5byhOgPll3UfHy2Zt+Q7qtCP+q6Pnm\n\tvlkQ==","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=8f+67xMVVa2AYJnZOwRLL/u44JkQLpgyU2YPjLkSaBk=;\n\tb=rLeiWOInS+Q8c9YFsq+b+A5n75HaOK2D8wqWi9RfkpOVEzkvf1lQxfLwMrDv6xd9F+\n\td0KXpWR1eegI/6HWd9untAJm24dmYMOMtBRI/ooEmGycdBPDZvoCh276b2m824pwv+aT\n\tCtcmGnI6kg3H11sI1pxoJzBJJnq5i6dgFk53ndLh9DYM7W+XoS7PKBOudacyRY5FnbSc\n\tZ1jGdF3egxO24lKOAbnY1pehSb2a9a/usV3sRk8MHtX0tAjPcoCnkxG4NEHklBNP1wns\n\t+bA3KWUUytsAlYKwZprKkqDbqsBG7Fu3n5XN653z8vRgxJhJgi1jsgk3o1dB3BD83oZg\n\t21ZA==","X-Gm-Message-State":"AOAM532xcxGNjMs1PCs9CsZF5SB3II4B0xKgWDF70YyqirqPEOXOICHC\n\tEQiPt49jXCKxSFTnR5MZAAWFqBtB4zg4CM/KwzONzw==","X-Google-Smtp-Source":"ABdhPJy6rS1knFriYW+eWv+1xcZj6cj2GQzFgSEhdGgApDqF8YwZeTXSoIdtFyxduPT3zW1DewvVH7cq6lKbjpLbDYg=","X-Received":"by 2002:a2e:9b97:: with SMTP id z23mr6548469lji.1.1600191617886; \n\tTue, 15 Sep 2020 10:40:17 -0700 (PDT)","MIME-Version":"1.0","References":"<20200908074913.109244-1-naush@raspberrypi.com>\n\t<20200908074913.109244-13-naush@raspberrypi.com>\n\t<876808e9-66ac-f009-9d4b-d6bbabd4fd73@ideasonboard.com>\n\t<20200915145534.GA1850958@oden.dyn.berto.se>\n\t\n\t<20200915173124.GB1850958@oden.dyn.berto.se>","In-Reply-To":"<20200915173124.GB1850958@oden.dyn.berto.se>","From":"Naushir Patuck ","Date":"Tue, 15 Sep 2020 18:40:00 +0100","Message-ID":"","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","Subject":"Re: [libcamera-devel] [PATCH v7 12/12] pipeline: ipa: raspberrypi:\n\tHandle any externally allocated FrameBuffer","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":12529,"web_url":"https://patchwork.libcamera.org/comment/12529/","msgid":"<20200915175350.GG26029@pendragon.ideasonboard.com>","date":"2020-09-15T17:53:50","subject":"Re: [libcamera-devel] [PATCH v7 12/12] pipeline: ipa: raspberrypi:\n\tHandle any externally allocated FrameBuffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Tue, Sep 15, 2020 at 06:40:00PM +0100, Naushir Patuck wrote:\n> On Tue, 15 Sep 2020 at 18:31, Niklas Söderlund wrote:\n> > On 2020-09-15 16:18:07 +0100, Naushir Patuck wrote:\n> > > On Tue, 15 Sep 2020 at 15:55, Niklas Söderlund wrote:\n> > > > On 2020-09-15 14:59:23 +0100, Kieran Bingham wrote:\n> > > > > On 08/09/2020 08:49, Naushir Patuck wrote:\n> > > > > > Handle the case where a FrameBuffer that has been externally allocated\n> > > > > > (i.e. not through the v4l2 video device) is passed into a Request.\n> > > > > >\n> > > > > > We must store the buffer pointer in the stream internal buffer list to\n> > > > > > identify when used.\n> > > > > >\n> > > > > > Signed-off-by: Naushir Patuck \n> > > > > > ---\n> > > > > > include/libcamera/ipa/raspberrypi.h | 11 ++--\n> > > > > > src/ipa/raspberrypi/raspberrypi.cpp | 6 +-\n> > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 64 +++++++++++++------\n> > > > > > .../pipeline/raspberrypi/rpi_stream.cpp | 30 ++++++++-\n> > > > > > .../pipeline/raspberrypi/rpi_stream.h | 3 +\n> > > > > > 5 files changed, 85 insertions(+), 29 deletions(-)\n> > > > > >\n> > > > > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > > > > > index 262fc6f3..dd6ebeac 100644\n> > > > > > --- a/include/libcamera/ipa/raspberrypi.h\n> > > > > > +++ b/include/libcamera/ipa/raspberrypi.h\n> > > > > > @@ -28,11 +28,12 @@ enum RPiOperations {\n> > > > > > RPI_IPA_EVENT_QUEUE_REQUEST,\n> > > > > > };\n> > > > > >\n> > > > > > -enum RPiIpaMask {\n> > > > > > - ID = 0x0ffff,\n> > > > > > - STATS = 0x10000,\n> > > > > > - EMBEDDED_DATA = 0x20000,\n> > > > > > - BAYER_DATA = 0x40000\n> > > > > > +enum RPiBufferMask {\n> > > > > > + ID = 0x00ffff,\n> > > > > > + STATS = 0x010000,\n> > > > > > + EMBEDDED_DATA = 0x020000,\n> > > > > > + BAYER_DATA = 0x040000,\n> > > > > > + EXTERNAL_BUFFER = 0x100000,\n> > > > > > };\n> > > > > >\n> > > > > > /* Size of the LS grid allocation. */\n> > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > > index 688d2efc..0555cc4e 100644\n> > > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > > @@ -352,7 +352,7 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> > > > > >\n> > > > > > IPAOperationData op;\n> > > > > > op.operation = RPI_IPA_ACTION_STATS_METADATA_COMPLETE;\n> > > > > > - op.data = { bufferId & RPiIpaMask::ID };\n> > > > > > + op.data = { bufferId & RPiBufferMask::ID };\n> > > > > > op.controls = { libcameraMetadata_ };\n> > > > > > queueFrameAction.emit(0, op);\n> > > > > > break;\n> > > > > > @@ -373,7 +373,7 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> > > > > > /* Ready to push the input buffer into the ISP. */\n> > > > > > IPAOperationData op;\n> > > > > > op.operation = RPI_IPA_ACTION_RUN_ISP;\n> > > > > > - op.data = { bayerbufferId & RPiIpaMask::ID };\n> > > > > > + op.data = { bayerbufferId & RPiBufferMask::ID };\n> > > > > > queueFrameAction.emit(0, op);\n> > > > > > break;\n> > > > > > }\n> > > > > > @@ -700,7 +700,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> > > > > > {\n> > > > > > IPAOperationData op;\n> > > > > > op.operation = RPI_IPA_ACTION_EMBEDDED_COMPLETE;\n> > > > > > - op.data = { bufferId & RPiIpaMask::ID };\n> > > > > > + op.data = { bufferId & RPiBufferMask::ID };\n> > > > > > queueFrameAction.emit(0, op);\n> > > > > > }\n> > > > > >\n> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > index 5621f182..f5e0d1cd 100644\n> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > @@ -151,6 +151,7 @@ public:\n> > > > > >\n> > > > > > void clearIncompleteRequests();\n> > > > > > void handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);\n> > > > > > + void handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);\n> > > > > > void handleState();\n> > > > > >\n> > > > > > CameraSensor *sensor_;\n> > > > > > @@ -725,23 +726,32 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> > > > > >\n> > > > > > /* Push all buffers supplied in the Request to the respective streams. */\n> > > > > > for (auto stream : data->streams_) {\n> > > > > > - if (stream->isExternal()) {\n> > > > > > - FrameBuffer *buffer = request->findBuffer(stream);\n> > > > > > + if (!stream->isExternal())\n> > > > > > + continue;\n> > > > > > +\n> > > > > > + FrameBuffer *buffer = request->findBuffer(stream);\n> > > > > > + if (buffer && stream->getBufferId(buffer) == -1) {\n> > > > > > /*\n> > > > > > - * If no buffer is provided by the request for this stream, we\n> > > > > > - * queue a nullptr to the stream to signify that it must use an\n> > > > > > - * internally allocated buffer for this capture request. This\n> > > > > > - * buffer will not be given back to the application, but is used\n> > > > > > - * to support the internal pipeline flow.\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> > > > > > + * This buffer is not recognised, so it must have been allocated\n> > > > > > + * outside the v4l2 device. Store it in the stream buffer list\n> > > > > > + * so we can track it.\n> > > > > > */\n> > > > > > - int ret = stream->queueBuffer(buffer);\n> > > > > > - if (ret)\n> > > > > > - return ret;\n> > > > > > + stream->setExternalBuffer(buffer);\n> > > > > > }\n> > > > > > + /*\n> > > > > > + * If no buffer is provided by the request for this stream, we\n> > > > > > + * queue a nullptr to the stream to signify that it must use an\n> > > > > > + * internally allocated buffer for this capture request. This\n> > > > > > + * buffer will not be given back to the application, but is used\n> > > > > > + * to support the internal pipeline flow.\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> > > > > > /* Push the request to the back of the queue. */\n> > > > > > @@ -915,8 +925,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > > > > > * Pass the stats and embedded data buffers to the IPA. No other\n> > > > > > * buffers need to be passed.\n> > > > > > */\n> > > > > > - mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPiIpaMask::STATS);\n> > > > > > - mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), RPiIpaMask::EMBEDDED_DATA);\n> > > > > > + mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPiBufferMask::STATS);\n> > > > > > + mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), RPiBufferMask::EMBEDDED_DATA);\n> > > > > >\n> > > > > > return 0;\n> > > > > > }\n> > > > > > @@ -1219,7 +1229,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n> > > > > > if (stream == &isp_[Isp::Stats]) {\n> > > > > > IPAOperationData op;\n> > > > > > op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;\n> > > > > > - op.data = { RPiIpaMask::STATS | static_cast(index) };\n> > > > > > + op.data = { RPiBufferMask::STATS | static_cast(index) };\n> > > > > > ipa_->processEvent(op);\n> > > > > > } else {\n> > > > > > /* Any other ISP output can be handed back to the application now. */\n> > > > > > @@ -1293,6 +1303,11 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre\n> > > > > > Request *request = requestQueue_.front();\n> > > > > >\n> > > > > > if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {\n> > > > > > + /*\n> > > > > > + * Check if this is an externally provided buffer, and if\n> > > > > > + * so, we must stop tracking it in the pipeline handler.\n> > > > > > + */\n> > > > > > + handleExternalBuffer(buffer, stream);\n> > > > > > /*\n> > > > > > * Tag the buffer as completed, returning it to the\n> > > > > > * application.\n> > > > > > @@ -1311,6 +1326,17 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre\n> > > > > > }\n> > > > > > }\n> > > > > >\n> > > > > > +void RPiCameraData::handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)\n> > > > > > +{\n> > > > > > + unsigned int id = stream->getBufferId(buffer);\n> > > > > > +\n> > > > > > + if (!(id & RPiBufferMask::EXTERNAL_BUFFER))\n> > > > > > + return;\n> > > > > > +\n> > > > > > + /* Stop the Stream object from tracking the buffer. */\n> > > > > > + stream->removeExternalBuffer(buffer);\n> > > > > > +}\n> > > > > > +\n> > > > > > void RPiCameraData::handleState()\n> > > > > > {\n> > > > > > switch (state_) {\n> > > > > > @@ -1443,8 +1469,8 @@ void RPiCameraData::tryRunPipeline()\n> > > > > > << \" Embedded buffer id: \" << embeddedIndex;\n> > > > > >\n> > > > > > op.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE;\n> > > > > > - op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex,\n> > > > > > - RPiIpaMask::BAYER_DATA | bayerIndex };\n> > > > > > + op.data = { RPiBufferMask::EMBEDDED_DATA | embeddedIndex,\n> > > > > > + RPiBufferMask::BAYER_DATA | bayerIndex };\n> > > > > > ipa_->processEvent(op);\n> > > > > > }\n> > > > > >\n> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > > > > index 879e25ba..c09f14c9 100644\n> > > > > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > > > > @@ -6,6 +6,8 @@\n> > > > > > */\n> > > > > > #include \"rpi_stream.h\"\n> > > > > >\n> > > > > > +#include \n> > > > > > +\n> > > > > > #include \"libcamera/internal/log.h\"\n> > > > > >\n> > > > > > namespace libcamera {\n> > > > > > @@ -44,8 +46,13 @@ bool RPiStream::isExternal() const\n> > > > > >\n> > > > > > void RPiStream::setExportedBuffers(std::vector> *buffers)\n> > > > > > {\n> > > > > > - for (auto const &buffer : *buffers)\n> > > > > > - bufferMap_.emplace(id_++, buffer.get());\n> > > > > > + /* Ensure we are using a sensible number of buffers. */\n> > > > > > + ASSERT(id_ < RPiBufferMask::ID);\n> > > > > > +\n> > > > > > + for (auto const &buffer : *buffers) {\n> > > > > > + bufferMap_.emplace(id_ & RPiBufferMask::ID, buffer.get());\n> > > > > > + id_++;\n> > > > > > + }\n> > > > >\n> > > > > I half wonder if that assert shouldn't come 'after' the updates to the\n> > > > > bufferMap to catch if the id_ has overflowed after increase.\n> > > > >\n> > > > > But maybe this is a tiny corner case and not expected anyway.\n> > > > >\n> > > > >\n> > > > > > }\n> > > > > >\n> > > > > > const BufferMap &RPiStream::getBuffers() const\n> > > > > > @@ -68,6 +75,25 @@ int RPiStream::getBufferId(FrameBuffer *buffer) const\n> > > > > > return it->first;\n> > > > > > }\n> > > > > >\n> > > > > > +void RPiStream::setExternalBuffer(FrameBuffer *buffer)\n> > > > > > +{\n> > > > > > + /* Ensure we are using a sensible number of buffers. */\n> > > > > > + ASSERT(id_ < RPiBufferMask::ID);\n> > > > > > +\n> > > > > > + bufferMap_.emplace(RPiBufferMask::EXTERNAL_BUFFER | (id_ & RPiBufferMask::ID),\n> > > > > > + buffer);\n> > > > > > + id_++;\n> > > > >\n> > > > > Oh, ... is there no reduction of the id_?\n> > > > >\n> > > > > What happens if an application continually provides an external buffer\n> > > > > ... won't that constantly increase this id_ with every frame?\n> > > > >\n> > > > > Are the ID's re-used? or is it just that the stream doesn't generate a\n> > > > > new ID for each usage of an external buffer (i.e. it really would have\n> > > > > to be more than 65535 entirely different buffers...)\n> > > > >\n> > > > > I guess this can be easily validated by setting RPiBufferMask::ID to\n> > > > > 0xFF or even 0xF to see if it hits any assertions quickly\n> > > > >\n> > > > > As long as this doesn't leak and cause an assertion/overflow after\n> > > > > 0xFFFF/RPiBufferMask::ID requests are queued ...\n> > > >\n> > > > This is a very good observation that I missed in my review. Good thing\n> > > > you looked at it as I was about to merge this series.\n> > > >\n> > > > Naush would it be possible to rework this logic slightly to makesure the\n> > > > id_ used can rollover while at the same time is not overwriting an\n> > > > existing mapping? With this solved I think we are ready to merge this\n> > > > series \\o/.\n> > >\n> > > Currently we will hit an assertion if id goes over 0xffff as it is\n> > > never decremented. If this is going to be a problem (and it sounds\n> > > like it will be for android that only ever uses external buffers?), I\n> > > will think about how to rework this to allow for reusing a previously\n> > > returned id.\n> >\n> > How about something like this,\n> >\n> > /* Find next free buffer id. */\n> > unsigned int id = (id_ + 1) % RPiBufferMask::ID;\n> > while (id != id_ && bufferMap_.find(id) != bufferMap_.end()) {\n> > id = (id + 1) % RPiBufferMask::ID;\n> >\n> > /* No free id could be found, this really should not happen. */\n> > ASSERT(id != id_);\n> >\n> > id_ = id;\n> >\n> > Of course untested ;-)\n> \n> Something along those lines popped into my head as well. However, I\n> worry about the efficiency of map::find() on every frame, across all\n> ids if the map becomes dense with a large number of buffers. Instead,\n> perhaps track recyclable ids in a separate queue so we can avoid the\n> costly lookup?\n\nSomething more efficient than the above code would indeed be nice. Do we\nhave an upper bound on the number of IDs we will need concurrently ? If\nit's low enough a bitmask combined with ffs() (and similar functions)\nshould be fairly straightforward. Otherwise, \"efficient ID allocator\"\nmay be a good query for a search engine :-)\n\n> > > > > Reviewed-by: Kieran Bingham \n> > > > >\n> > > > > > +}\n> > > > > > +\n> > > > > > +void RPiStream::removeExternalBuffer(FrameBuffer *buffer)\n> > > > > > +{\n> > > > > > + int id = getBufferId(buffer);\n> > > > > > +\n> > > > > > + /* Ensure we have this buffer in the stream, and it is marked external. */\n> > > > > > + ASSERT(id != -1 && (id & RPiBufferMask::EXTERNAL_BUFFER));\n> > > > > > + bufferMap_.erase(id);\n> > > > > > +}\n> > > > > > +\n> > > > > > int RPiStream::prepareBuffers(unsigned int count)\n> > > > > > {\n> > > > > > int ret;\n> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > > > > > index ed517c22..8b23c4b2 100644\n> > > > > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > > > > > @@ -51,6 +51,9 @@ public:\n> > > > > > const BufferMap &getBuffers() const;\n> > > > > > int getBufferId(FrameBuffer *buffer) const;\n> > > > > >\n> > > > > > + void setExternalBuffer(FrameBuffer *buffer);\n> > > > > > + void removeExternalBuffer(FrameBuffer *buffer);\n> > > > > > +\n> > > > > > int prepareBuffers(unsigned int count);\n> > > > > > int queueBuffer(FrameBuffer *buffer);\n> > > > > > void returnBuffer(FrameBuffer *buffer);","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 F3B9FBF01C\n\tfor ;\n\tTue, 15 Sep 2020 17:54:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7F0C662E13;\n\tTue, 15 Sep 2020 19:54:22 +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 D8A7C603EE\n\tfor ;\n\tTue, 15 Sep 2020 19:54:20 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 12DBFFD8;\n\tTue, 15 Sep 2020 19:54:20 +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=\"WvXDycZG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600192460;\n\tbh=6w/KxjU2hwYXdrIHfPgKZ9ANSnifpDf5Ca/IsfuLLAQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WvXDycZG/vizDk1Lp5RiV0ZxMvQEW7Gc4VEaxhjTVTn0qhOrZ0VsDlbwqV009vRwB\n\t/XZvbZny8xZn81JeDEo7vUWN8rU4CoVNHkpf35o8yyYSE6rVXzgiYmbdBw9y4HJ0jc\n\tk8pO6TqRusdkJpfKQokr1Ym/8EaDlFMJKZInqKck=","Date":"Tue, 15 Sep 2020 20:53:50 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"<20200915175350.GG26029@pendragon.ideasonboard.com>","References":"<20200908074913.109244-1-naush@raspberrypi.com>\n\t<20200908074913.109244-13-naush@raspberrypi.com>\n\t<876808e9-66ac-f009-9d4b-d6bbabd4fd73@ideasonboard.com>\n\t<20200915145534.GA1850958@oden.dyn.berto.se>\n\t\n\t<20200915173124.GB1850958@oden.dyn.berto.se>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v7 12/12] pipeline: ipa: raspberrypi:\n\tHandle any externally allocated FrameBuffer","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\" "}}]