[{"id":28867,"web_url":"https://patchwork.libcamera.org/comment/28867/","msgid":"<21e0c6c6-35bd-42ec-86fe-480285082cdf@ideasonboard.com>","date":"2024-03-05T11:50:11","subject":"Re: [PATCH 3/5] libcamera: rkisp1: Replace usage of RkISP1FrameInfo","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 21/02/2024 17:40, Jacopo Mondi wrote:\n> Now that the pipeline handler can create a private derived class of\n> Request::Private, use it to store the pipeline specific data.\n>\n> In the case of RkISP1 we associate the statistic and paramters buffers\n> with a Request.\n>\n> As the IPA sends notifications for paramters and statistics buffer by\n> identifying them by frame id, associate the frame ids and the Request\n> in the RkISP1CameraData class.\n>\n> This replaces the functionalities of RkISP1FrameInfo which can now be\n> removed.\n>\n> Signed-off-by: Jacopo Mondi \n> ---\n\n\nCode here looks fine to me so:\n\nReviewed-by: Daniel Scally \n\nand\n\nTested-by: Daniel Scally \n\n\nHowever I do wonder if the change in queueRequestDevice() to stop unconditionally queuing frames to \nthe IPA should go into its own commit before this one - it's a good change but also could stand \nalone. Up to you though.\n\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 358 ++++++++---------------\n> 1 file changed, 128 insertions(+), 230 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index e981e60758f7..f2163f528251 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -49,76 +49,63 @@ namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(RkISP1)\n> \n> -class PipelineHandlerRkISP1;\n> -class RkISP1CameraData;\n> +class RkISP1Request : public Request::Private\n> +{\n> +public:\n> +\tRkISP1Request(Camera *camera)\n> +\t\t: Request::Private(camera)\n> +\t{\n> +\t}\n> \n> -struct RkISP1FrameInfo {\n> -\tunsigned int frame;\n> -\tRequest *request;\n> +\tbool hasPendingBuffers(bool isRaw) const;\n> \n> -\tFrameBuffer *paramBuffer;\n> \tFrameBuffer *statBuffer;\n> -\tFrameBuffer *mainPathBuffer;\n> -\tFrameBuffer *selfPathBuffer;\n> +\tFrameBuffer *paramBuffer;\n> +\n> +\t/* The frame number this request is associated with. */\n> +\tunsigned int frame;\n> \n> \tbool paramDequeued;\n> \tbool metadataProcessed;\n> };\n> \n> -class RkISP1Frames\n> -{\n> -public:\n> -\tRkISP1Frames(PipelineHandler *pipe);\n> -\n> -\tRkISP1FrameInfo *create(const RkISP1CameraData *data, Request *request,\n> -\t\t\t\tbool isRaw);\n> -\tint destroy(unsigned int frame);\n> -\tvoid clear();\n> -\n> -\tRkISP1FrameInfo *find(unsigned int frame);\n> -\tRkISP1FrameInfo *find(FrameBuffer *buffer);\n> -\tRkISP1FrameInfo *find(Request *request);\n> -\n> -private:\n> -\tPipelineHandlerRkISP1 *pipe_;\n> -\tstd::map frameInfo_;\n> -};\n> -\n> -class RkISP1Request : public Request::Private\n> +bool RkISP1Request::hasPendingBuffers(bool isRaw) const\n> {\n> -public:\n> -\tRkISP1Request(Camera *camera)\n> -\t\t: Request::Private(camera)\n> -\t{\n> -\t}\n> -};\n> +\treturn Request::Private::hasPendingBuffers() ||\n> +\t !metadataProcessed || (!isRaw && !paramDequeued);\n> +}\n> \n> +class PipelineHandlerRkISP1;\n> class RkISP1CameraData : public Camera::Private\n> {\n> public:\n> \tRkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath,\n> \t\t\t RkISP1SelfPath *selfPath)\n> -\t\t: Camera::Private(pipe), frame_(0), frameInfo_(pipe),\n> -\t\t mainPath_(mainPath), selfPath_(selfPath)\n> +\t\t: Camera::Private(pipe), frame_(0), mainPath_(mainPath),\n> +\t\t selfPath_(selfPath)\n> \t{\n> \t}\n> \n> \tPipelineHandlerRkISP1 *pipe();\n> \tint loadIPA(unsigned int hwRevision);\n> \n> +\tvoid addRequest(RkISP1Request *request);\n> +\n> \tStream mainPathStream_;\n> \tStream selfPathStream_;\n> \tstd::unique_ptr sensor_;\n> \tstd::unique_ptr delayedCtrls_;\n> \tunsigned int frame_;\n> \tstd::vector ipaBuffers_;\n> -\tRkISP1Frames frameInfo_;\n> \n> \tRkISP1MainPath *mainPath_;\n> \tRkISP1SelfPath *selfPath_;\n> \n> \tstd::unique_ptr ipa_;\n> \n> +\t/* Associate a frame id with a Request. */\n> +\tstd::map requestMap_;\n> +\n> private:\n> \tvoid paramFilled(unsigned int frame);\n> \tvoid setSensorControls(unsigned int frame,\n> @@ -181,13 +168,17 @@ private:\n> \t\treturn static_cast(camera->_d());\n> \t}\n> \n> +\tRkISP1Request *cameraRequest(Request *request)\n> +\t{\n> +\t\treturn static_cast(request->_d());\n> +\t}\n> +\n> \tfriend RkISP1CameraData;\n> -\tfriend RkISP1Frames;\n> \n> \tint initLinks(Camera *camera, const CameraSensor *sensor,\n> \t\t const RkISP1CameraConfiguration &config);\n> \tint createCamera(MediaEntity *sensor);\n> -\tvoid tryCompleteRequest(RkISP1FrameInfo *info);\n> +\tvoid tryCompleteRequest(RkISP1Request *request);\n> \tvoid bufferReady(FrameBuffer *buffer);\n> \tvoid paramReady(FrameBuffer *buffer);\n> \tvoid statReady(FrameBuffer *buffer);\n> @@ -218,129 +209,6 @@ private:\n> \tconst MediaPad *ispSink_;\n> };\n> \n> -RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)\n> -\t: pipe_(static_cast(pipe))\n> -{\n> -}\n> -\n> -RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *request,\n> -\t\t\t\t bool isRaw)\n> -{\n> -\tunsigned int frame = data->frame_;\n> -\n> -\tFrameBuffer *paramBuffer = nullptr;\n> -\tFrameBuffer *statBuffer = nullptr;\n> -\n> -\tif (!isRaw) {\n> -\t\tif (pipe_->availableParamBuffers_.empty()) {\n> -\t\t\tLOG(RkISP1, Error) << \"Parameters buffer underrun\";\n> -\t\t\treturn nullptr;\n> -\t\t}\n> -\n> -\t\tif (pipe_->availableStatBuffers_.empty()) {\n> -\t\t\tLOG(RkISP1, Error) << \"Statistic buffer underrun\";\n> -\t\t\treturn nullptr;\n> -\t\t}\n> -\n> -\t\tparamBuffer = pipe_->availableParamBuffers_.front();\n> -\t\tpipe_->availableParamBuffers_.pop();\n> -\n> -\t\tstatBuffer = pipe_->availableStatBuffers_.front();\n> -\t\tpipe_->availableStatBuffers_.pop();\n> -\t}\n> -\n> -\tFrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> -\tFrameBuffer *selfPathBuffer = request->findBuffer(&data->selfPathStream_);\n> -\n> -\tRkISP1FrameInfo *info = new RkISP1FrameInfo;\n> -\n> -\tinfo->frame = frame;\n> -\tinfo->request = request;\n> -\tinfo->paramBuffer = paramBuffer;\n> -\tinfo->mainPathBuffer = mainPathBuffer;\n> -\tinfo->selfPathBuffer = selfPathBuffer;\n> -\tinfo->statBuffer = statBuffer;\n> -\tinfo->paramDequeued = false;\n> -\tinfo->metadataProcessed = false;\n> -\n> -\tframeInfo_[frame] = info;\n> -\n> -\treturn info;\n> -}\n> -\n> -int RkISP1Frames::destroy(unsigned int frame)\n> -{\n> -\tRkISP1FrameInfo *info = find(frame);\n> -\tif (!info)\n> -\t\treturn -ENOENT;\n> -\n> -\tpipe_->availableParamBuffers_.push(info->paramBuffer);\n> -\tpipe_->availableStatBuffers_.push(info->statBuffer);\n> -\n> -\tframeInfo_.erase(info->frame);\n> -\n> -\tdelete info;\n> -\n> -\treturn 0;\n> -}\n> -\n> -void RkISP1Frames::clear()\n> -{\n> -\tfor (const auto &entry : frameInfo_) {\n> -\t\tRkISP1FrameInfo *info = entry.second;\n> -\n> -\t\tpipe_->availableParamBuffers_.push(info->paramBuffer);\n> -\t\tpipe_->availableStatBuffers_.push(info->statBuffer);\n> -\n> -\t\tdelete info;\n> -\t}\n> -\n> -\tframeInfo_.clear();\n> -}\n> -\n> -RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)\n> -{\n> -\tauto itInfo = frameInfo_.find(frame);\n> -\n> -\tif (itInfo != frameInfo_.end())\n> -\t\treturn itInfo->second;\n> -\n> -\tLOG(RkISP1, Fatal) << \"Can't locate info from frame\";\n> -\n> -\treturn nullptr;\n> -}\n> -\n> -RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n> -{\n> -\tfor (auto &itInfo : frameInfo_) {\n> -\t\tRkISP1FrameInfo *info = itInfo.second;\n> -\n> -\t\tif (info->paramBuffer == buffer ||\n> -\t\t info->statBuffer == buffer ||\n> -\t\t info->mainPathBuffer == buffer ||\n> -\t\t info->selfPathBuffer == buffer)\n> -\t\t\treturn info;\n> -\t}\n> -\n> -\tLOG(RkISP1, Fatal) << \"Can't locate info from buffer\";\n> -\n> -\treturn nullptr;\n> -}\n> -\n> -RkISP1FrameInfo *RkISP1Frames::find(Request *request)\n> -{\n> -\tfor (auto &itInfo : frameInfo_) {\n> -\t\tRkISP1FrameInfo *info = itInfo.second;\n> -\n> -\t\tif (info->request == request)\n> -\t\t\treturn info;\n> -\t}\n> -\n> -\tLOG(RkISP1, Fatal) << \"Can't locate info from request\";\n> -\n> -\treturn nullptr;\n> -}\n> -\n> PipelineHandlerRkISP1 *RkISP1CameraData::pipe()\n> {\n> \treturn static_cast(Camera::Private::pipe());\n> @@ -391,23 +259,34 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> \treturn 0;\n> }\n> \n> +void RkISP1CameraData::addRequest(RkISP1Request *request)\n> +{\n> +\t/* Associate the request and the frame number. */\n> +\trequest->frame = frame_;\n> +\trequestMap_[frame_] = request;\n> +\tframe_++;\n> +}\n> +\n> void RkISP1CameraData::paramFilled(unsigned int frame)\n> {\n> \tPipelineHandlerRkISP1 *pipe = RkISP1CameraData::pipe();\n> -\tRkISP1FrameInfo *info = frameInfo_.find(frame);\n> -\tif (!info)\n> -\t\treturn;\n> +\tRkISP1Request *request = requestMap_.at(frame);\n> +\tASSERT(request);\n> \n> -\tinfo->paramBuffer->_d()->metadata().planes()[0].bytesused =\n> +\trequest->paramBuffer->_d()->metadata().planes()[0].bytesused =\n> \t\tsizeof(struct rkisp1_params_cfg);\n> -\tpipe->param_->queueBuffer(info->paramBuffer);\n> -\tpipe->stat_->queueBuffer(info->statBuffer);\n> -\n> -\tif (info->mainPathBuffer)\n> -\t\tmainPath_->queueBuffer(info->mainPathBuffer);\n> -\n> -\tif (selfPath_ && info->selfPathBuffer)\n> -\t\tselfPath_->queueBuffer(info->selfPathBuffer);\n> +\tpipe->param_->queueBuffer(request->paramBuffer);\n> +\tpipe->stat_->queueBuffer(request->statBuffer);\n> +\n> +\tFrameBuffer *mainPathBuffer =\n> +\t\trequest->_o()->findBuffer(&mainPathStream_);\n> +\tif (mainPathBuffer)\n> +\t\tmainPath_->queueBuffer(mainPathBuffer);\n> +\n> +\tFrameBuffer *selfPathBuffer =\n> +\t\trequest->_o()->findBuffer(&selfPathStream_);\n> +\tif (selfPath_ && selfPathBuffer)\n> +\t\tselfPath_->queueBuffer(selfPathBuffer);\n> }\n> \n> void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,\n> @@ -418,14 +297,13 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,\n> \n> void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n> {\n> -\tRkISP1FrameInfo *info = frameInfo_.find(frame);\n> -\tif (!info)\n> -\t\treturn;\n> +\tRkISP1Request *request = requestMap_.at(frame);\n> +\tASSERT(request);\n> \n> -\tinfo->request->metadata().merge(metadata);\n> -\tinfo->metadataProcessed = true;\n> +\trequest->_o()->metadata().merge(metadata);\n> +\trequest->metadataProcessed = true;\n> \n> -\tpipe()->tryCompleteRequest(info);\n> +\tpipe()->tryCompleteRequest(request);\n> }\n> \n> /* -----------------------------------------------------------------------------\n> @@ -1025,7 +903,7 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n> \t}\n> \n> \tASSERT(data->queuedRequests_.empty());\n> -\tdata->frameInfo_.clear();\n> +\tdata->requestMap_.clear();\n> \n> \tfreeBuffers(camera);\n> \n> @@ -1042,24 +920,60 @@ std::unique_ptr PipelineHandlerRkISP1::createRequestDevice(Camera *came\n> int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> {\n> \tRkISP1CameraData *data = cameraData(camera);\n> +\tRkISP1Request *rkisp1Request = cameraRequest(request);\n> \n> -\tRkISP1FrameInfo *info = data->frameInfo_.create(data, request, isRaw_);\n> -\tif (!info)\n> -\t\treturn -ENOENT;\n> +\tdata->addRequest(rkisp1Request);\n> \n> -\tdata->ipa_->queueRequest(data->frame_, request->controls());\n> +\t/*\n> +\t * If we're operating in RAW mode (only one RAW stream is captured)\n> +\t * then we simply queue buffers to the video devices as we don't\n> +\t * need to run the IPA.\n> +\t */\n> \tif (isRaw_) {\n> -\t\tif (info->mainPathBuffer)\n> -\t\t\tdata->mainPath_->queueBuffer(info->mainPathBuffer);\n> +\t\tFrameBuffer *mainPathBuffer =\n> +\t\t\trequest->findBuffer(&data->mainPathStream_);\n> +\t\tif (mainPathBuffer)\n> +\t\t\tdata->mainPath_->queueBuffer(mainPathBuffer);\n> \n> -\t\tif (data->selfPath_ && info->selfPathBuffer)\n> -\t\t\tdata->selfPath_->queueBuffer(info->selfPathBuffer);\n> -\t} else {\n> -\t\tdata->ipa_->fillParamsBuffer(data->frame_,\n> -\t\t\t\t\t info->paramBuffer->cookie());\n> +\t\tFrameBuffer *selfPathBuffer =\n> +\t\t\trequest->findBuffer(&data->selfPathStream_);\n> +\t\tif (data->selfPath_ && selfPathBuffer)\n> +\t\t\tdata->selfPath_->queueBuffer(selfPathBuffer);\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\t/*\n> +\t * If we run the IPA we need to associate a parameters and a statistics\n> +\t * buffer with the Request and associate the request with the current\n> +\t * frame number.\n> +\t *\n> +\t * Associate the stat and frame buffers to a Request (if available)\n> +\t * and then run the IPA.\n> +\t */\n> +\tif (availableParamBuffers_.empty()) {\n> +\t\tLOG(RkISP1, Error) << \"Parameters buffer underrun\";\n> +\t\treturn -ENOENT;\n> \t}\n> \n> -\tdata->frame_++;\n> +\tif (availableStatBuffers_.empty()) {\n> +\t\tLOG(RkISP1, Error) << \"Statistic buffer underrun\";\n> +\t\treturn -ENOENT;\n> +\t}\n> +\n> +\trkisp1Request->paramBuffer = availableParamBuffers_.front();\n> +\trkisp1Request->paramDequeued = false;\n> +\trkisp1Request->paramBuffer->_d()->setRequest(request);\n> +\tavailableParamBuffers_.pop();\n> +\n> +\trkisp1Request->statBuffer = availableStatBuffers_.front();\n> +\trkisp1Request->metadataProcessed = false;\n> +\trkisp1Request->statBuffer->_d()->setRequest(request);\n> +\tavailableStatBuffers_.pop();\n> +\n> +\tdata->ipa_->queueRequest(rkisp1Request->frame, request->controls());\n> +\tdata->ipa_->fillParamsBuffer(rkisp1Request->frame,\n> +\t\t\t\t rkisp1Request->paramBuffer->cookie());\n> \n> \treturn 0;\n> }\n> @@ -1251,37 +1165,28 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> * Buffer Handling\n> */\n> \n> -void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)\n> +void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1Request *request)\n> {\n> \tRkISP1CameraData *data = cameraData(activeCamera_);\n> -\tRequest *request = info->request;\n> -\n> -\tif (request->hasPendingBuffers())\n> -\t\treturn;\n> -\n> -\tif (!info->metadataProcessed)\n> -\t\treturn;\n> \n> -\tif (!isRaw_ && !info->paramDequeued)\n> +\tif (request->hasPendingBuffers(isRaw_))\n> \t\treturn;\n> \n> -\tdata->frameInfo_.destroy(info->frame);\n> +\t/* Return the stat and param buffers to the pipeline. */\n> +\tavailableParamBuffers_.push(request->paramBuffer);\n> +\tavailableStatBuffers_.push(request->statBuffer);\n> +\tdata->requestMap_.erase(request->frame);\n> \n> -\tcompleteRequest(request);\n> +\tcompleteRequest(request->_o());\n> }\n> \n> void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> {\n> \tASSERT(activeCamera_);\n> \tRkISP1CameraData *data = cameraData(activeCamera_);\n> -\n> -\tRkISP1FrameInfo *info = data->frameInfo_.find(buffer);\n> -\tif (!info)\n> -\t\treturn;\n> +\tRkISP1Request *request = cameraRequest(buffer->request());\n> \n> \tconst FrameMetadata &metadata = buffer->metadata();\n> -\tRequest *request = buffer->request();\n> -\n> \tif (metadata.status != FrameMetadata::FrameCancelled) {\n> \t\t/*\n> \t\t * Record the sensor's timestamp in the request metadata.\n> @@ -1289,55 +1194,48 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> \t\t * \\todo The sensor timestamp should be better estimated by connecting\n> \t\t * to the V4L2Device::frameStart signal.\n> \t\t */\n> -\t\trequest->metadata().set(controls::SensorTimestamp,\n> -\t\t\t\t\tmetadata.timestamp);\n> +\t\trequest->_o()->metadata().set(controls::SensorTimestamp,\n> +\t\t\t\t\t\t metadata.timestamp);\n> \n> \t\tif (isRaw_) {\n> \t\t\tconst ControlList &ctrls =\n> \t\t\t\tdata->delayedCtrls_->get(metadata.sequence);\n> -\t\t\tdata->ipa_->processStatsBuffer(info->frame, 0, ctrls);\n> +\t\t\tdata->ipa_->processStatsBuffer(request->frame, 0, ctrls);\n> \t\t}\n> \t} else {\n> \t\tif (isRaw_)\n> -\t\t\tinfo->metadataProcessed = true;\n> +\t\t\trequest->metadataProcessed = true;\n> \t}\n> \n> -\tcompleteBuffer(request, buffer);\n> -\ttryCompleteRequest(info);\n> +\tcompleteBuffer(request->_o(), buffer);\n> +\ttryCompleteRequest(request);\n> }\n> \n> void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)\n> {\n> \tASSERT(activeCamera_);\n> -\tRkISP1CameraData *data = cameraData(activeCamera_);\n> +\tRkISP1Request *request = cameraRequest(buffer->request());\n> \n> -\tRkISP1FrameInfo *info = data->frameInfo_.find(buffer);\n> -\tif (!info)\n> -\t\treturn;\n> -\n> -\tinfo->paramDequeued = true;\n> -\ttryCompleteRequest(info);\n> +\trequest->paramDequeued = true;\n> +\ttryCompleteRequest(request);\n> }\n> \n> void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> {\n> \tASSERT(activeCamera_);\n> \tRkISP1CameraData *data = cameraData(activeCamera_);\n> -\n> -\tRkISP1FrameInfo *info = data->frameInfo_.find(buffer);\n> -\tif (!info)\n> -\t\treturn;\n> +\tRkISP1Request *request = cameraRequest(buffer->request());\n> \n> \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> -\t\tinfo->metadataProcessed = true;\n> -\t\ttryCompleteRequest(info);\n> +\t\trequest->metadataProcessed = true;\n> +\t\ttryCompleteRequest(request);\n> \t\treturn;\n> \t}\n> \n> \tif (data->frame_ <= buffer->metadata().sequence)\n> \t\tdata->frame_ = buffer->metadata().sequence + 1;\n> \n> -\tdata->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),\n> +\tdata->ipa_->processStatsBuffer(request->frame, request->statBuffer->cookie(),\n> \t\t\t\t data->delayedCtrls_->get(buffer->metadata().sequence));\n> }\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B08CAC326B\n\tfor ;\n\tTue, 5 Mar 2024 11:50:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 121E56286F;\n\tTue, 5 Mar 2024 12:50:15 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3113161C8D\n\tfor ;\n\tTue, 5 Mar 2024 12:50:14 +0100 (CET)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1A03422A\n\tfor ;\n\tTue, 5 Mar 2024 12:49:57 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"cskadQ4U\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709639397;\n\tbh=FmhZOAEMFNQhKYmdkmkZg2sj6UP5kAhjVkMJKd2+4Ns=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=cskadQ4UStDZ8Edz5ztTE1OPlzL39gQM20/fwvtizW6rFRRvwtxpzs7P/DwkEbnpe\n\tOYNLNLLKLbBVZUvg3XpERQcanXtAcKP52jdPKug3zzce1X2jS1XDSZdCeR09AdAHhM\n\tMQ6e88wPc3NI8u0ILSiu15SUxAOUA4NUbvu2b5PE=","Message-ID":"<21e0c6c6-35bd-42ec-86fe-480285082cdf@ideasonboard.com>","Date":"Tue, 5 Mar 2024 11:50:11 +0000","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 3/5] libcamera: rkisp1: Replace usage of RkISP1FrameInfo","Content-Language":"en-US","To":"libcamera-devel@lists.libcamera.org","References":"<20240221174015.52958-1-jacopo.mondi@ideasonboard.com>\n\t<20240221174015.52958-4-jacopo.mondi@ideasonboard.com>","From":"Dan Scally ","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"<20240221174015.52958-4-jacopo.mondi@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28882,"web_url":"https://patchwork.libcamera.org/comment/28882/","msgid":"<20240306154814.4w5ebb7rrjiqquru@jasper>","date":"2024-03-06T15:48:14","subject":"Re: [PATCH 3/5] libcamera: rkisp1: Replace usage of RkISP1FrameInfo","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Feb 21, 2024 at 06:40:11PM +0100, Jacopo Mondi wrote:\n> Now that the pipeline handler can create a private derived class of\n> Request::Private, use it to store the pipeline specific data.\n> \n> In the case of RkISP1 we associate the statistic and paramters buffers\n> with a Request.\n> \n> As the IPA sends notifications for paramters and statistics buffer by\n> identifying them by frame id, associate the frame ids and the Request\n> in the RkISP1CameraData class.\n> \n> This replaces the functionalities of RkISP1FrameInfo which can now be\n> removed.\n> \n> Signed-off-by: Jacopo Mondi \n\nReviewed-by: Stefan Klug\n\nCheers,\nStefan\n\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 358 ++++++++---------------\n> 1 file changed, 128 insertions(+), 230 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index e981e60758f7..f2163f528251 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -49,76 +49,63 @@ namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(RkISP1)\n> \n> -class PipelineHandlerRkISP1;\n> -class RkISP1CameraData;\n> +class RkISP1Request : public Request::Private\n> +{\n> +public:\n> +\tRkISP1Request(Camera *camera)\n> +\t\t: Request::Private(camera)\n> +\t{\n> +\t}\n> \n> -struct RkISP1FrameInfo {\n> -\tunsigned int frame;\n> -\tRequest *request;\n> +\tbool hasPendingBuffers(bool isRaw) const;\n> \n> -\tFrameBuffer *paramBuffer;\n> \tFrameBuffer *statBuffer;\n> -\tFrameBuffer *mainPathBuffer;\n> -\tFrameBuffer *selfPathBuffer;\n> +\tFrameBuffer *paramBuffer;\n> +\n> +\t/* The frame number this request is associated with. */\n> +\tunsigned int frame;\n> \n> \tbool paramDequeued;\n> \tbool metadataProcessed;\n> };\n> \n> -class RkISP1Frames\n> -{\n> -public:\n> -\tRkISP1Frames(PipelineHandler *pipe);\n> -\n> -\tRkISP1FrameInfo *create(const RkISP1CameraData *data, Request *request,\n> -\t\t\t\tbool isRaw);\n> -\tint destroy(unsigned int frame);\n> -\tvoid clear();\n> -\n> -\tRkISP1FrameInfo *find(unsigned int frame);\n> -\tRkISP1FrameInfo *find(FrameBuffer *buffer);\n> -\tRkISP1FrameInfo *find(Request *request);\n> -\n> -private:\n> -\tPipelineHandlerRkISP1 *pipe_;\n> -\tstd::map frameInfo_;\n> -};\n> -\n> -class RkISP1Request : public Request::Private\n> +bool RkISP1Request::hasPendingBuffers(bool isRaw) const\n> {\n> -public:\n> -\tRkISP1Request(Camera *camera)\n> -\t\t: Request::Private(camera)\n> -\t{\n> -\t}\n> -};\n> +\treturn Request::Private::hasPendingBuffers() ||\n> +\t !metadataProcessed || (!isRaw && !paramDequeued);\n> +}\n> \n> +class PipelineHandlerRkISP1;\n> class RkISP1CameraData : public Camera::Private\n> {\n> public:\n> \tRkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath,\n> \t\t\t RkISP1SelfPath *selfPath)\n> -\t\t: Camera::Private(pipe), frame_(0), frameInfo_(pipe),\n> -\t\t mainPath_(mainPath), selfPath_(selfPath)\n> +\t\t: Camera::Private(pipe), frame_(0), mainPath_(mainPath),\n> +\t\t selfPath_(selfPath)\n> \t{\n> \t}\n> \n> \tPipelineHandlerRkISP1 *pipe();\n> \tint loadIPA(unsigned int hwRevision);\n> \n> +\tvoid addRequest(RkISP1Request *request);\n> +\n> \tStream mainPathStream_;\n> \tStream selfPathStream_;\n> \tstd::unique_ptr sensor_;\n> \tstd::unique_ptr delayedCtrls_;\n> \tunsigned int frame_;\n> \tstd::vector ipaBuffers_;\n> -\tRkISP1Frames frameInfo_;\n> \n> \tRkISP1MainPath *mainPath_;\n> \tRkISP1SelfPath *selfPath_;\n> \n> \tstd::unique_ptr ipa_;\n> \n> +\t/* Associate a frame id with a Request. */\n> +\tstd::map requestMap_;\n> +\n> private:\n> \tvoid paramFilled(unsigned int frame);\n> \tvoid setSensorControls(unsigned int frame,\n> @@ -181,13 +168,17 @@ private:\n> \t\treturn static_cast(camera->_d());\n> \t}\n> \n> +\tRkISP1Request *cameraRequest(Request *request)\n> +\t{\n> +\t\treturn static_cast(request->_d());\n> +\t}\n> +\n> \tfriend RkISP1CameraData;\n> -\tfriend RkISP1Frames;\n> \n> \tint initLinks(Camera *camera, const CameraSensor *sensor,\n> \t\t const RkISP1CameraConfiguration &config);\n> \tint createCamera(MediaEntity *sensor);\n> -\tvoid tryCompleteRequest(RkISP1FrameInfo *info);\n> +\tvoid tryCompleteRequest(RkISP1Request *request);\n> \tvoid bufferReady(FrameBuffer *buffer);\n> \tvoid paramReady(FrameBuffer *buffer);\n> \tvoid statReady(FrameBuffer *buffer);\n> @@ -218,129 +209,6 @@ private:\n> \tconst MediaPad *ispSink_;\n> };\n> \n> -RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)\n> -\t: pipe_(static_cast(pipe))\n> -{\n> -}\n> -\n> -RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *request,\n> -\t\t\t\t bool isRaw)\n> -{\n> -\tunsigned int frame = data->frame_;\n> -\n> -\tFrameBuffer *paramBuffer = nullptr;\n> -\tFrameBuffer *statBuffer = nullptr;\n> -\n> -\tif (!isRaw) {\n> -\t\tif (pipe_->availableParamBuffers_.empty()) {\n> -\t\t\tLOG(RkISP1, Error) << \"Parameters buffer underrun\";\n> -\t\t\treturn nullptr;\n> -\t\t}\n> -\n> -\t\tif (pipe_->availableStatBuffers_.empty()) {\n> -\t\t\tLOG(RkISP1, Error) << \"Statistic buffer underrun\";\n> -\t\t\treturn nullptr;\n> -\t\t}\n> -\n> -\t\tparamBuffer = pipe_->availableParamBuffers_.front();\n> -\t\tpipe_->availableParamBuffers_.pop();\n> -\n> -\t\tstatBuffer = pipe_->availableStatBuffers_.front();\n> -\t\tpipe_->availableStatBuffers_.pop();\n> -\t}\n> -\n> -\tFrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> -\tFrameBuffer *selfPathBuffer = request->findBuffer(&data->selfPathStream_);\n> -\n> -\tRkISP1FrameInfo *info = new RkISP1FrameInfo;\n> -\n> -\tinfo->frame = frame;\n> -\tinfo->request = request;\n> -\tinfo->paramBuffer = paramBuffer;\n> -\tinfo->mainPathBuffer = mainPathBuffer;\n> -\tinfo->selfPathBuffer = selfPathBuffer;\n> -\tinfo->statBuffer = statBuffer;\n> -\tinfo->paramDequeued = false;\n> -\tinfo->metadataProcessed = false;\n> -\n> -\tframeInfo_[frame] = info;\n> -\n> -\treturn info;\n> -}\n> -\n> -int RkISP1Frames::destroy(unsigned int frame)\n> -{\n> -\tRkISP1FrameInfo *info = find(frame);\n> -\tif (!info)\n> -\t\treturn -ENOENT;\n> -\n> -\tpipe_->availableParamBuffers_.push(info->paramBuffer);\n> -\tpipe_->availableStatBuffers_.push(info->statBuffer);\n> -\n> -\tframeInfo_.erase(info->frame);\n> -\n> -\tdelete info;\n> -\n> -\treturn 0;\n> -}\n> -\n> -void RkISP1Frames::clear()\n> -{\n> -\tfor (const auto &entry : frameInfo_) {\n> -\t\tRkISP1FrameInfo *info = entry.second;\n> -\n> -\t\tpipe_->availableParamBuffers_.push(info->paramBuffer);\n> -\t\tpipe_->availableStatBuffers_.push(info->statBuffer);\n> -\n> -\t\tdelete info;\n> -\t}\n> -\n> -\tframeInfo_.clear();\n> -}\n> -\n> -RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)\n> -{\n> -\tauto itInfo = frameInfo_.find(frame);\n> -\n> -\tif (itInfo != frameInfo_.end())\n> -\t\treturn itInfo->second;\n> -\n> -\tLOG(RkISP1, Fatal) << \"Can't locate info from frame\";\n> -\n> -\treturn nullptr;\n> -}\n> -\n> -RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n> -{\n> -\tfor (auto &itInfo : frameInfo_) {\n> -\t\tRkISP1FrameInfo *info = itInfo.second;\n> -\n> -\t\tif (info->paramBuffer == buffer ||\n> -\t\t info->statBuffer == buffer ||\n> -\t\t info->mainPathBuffer == buffer ||\n> -\t\t info->selfPathBuffer == buffer)\n> -\t\t\treturn info;\n> -\t}\n> -\n> -\tLOG(RkISP1, Fatal) << \"Can't locate info from buffer\";\n> -\n> -\treturn nullptr;\n> -}\n> -\n> -RkISP1FrameInfo *RkISP1Frames::find(Request *request)\n> -{\n> -\tfor (auto &itInfo : frameInfo_) {\n> -\t\tRkISP1FrameInfo *info = itInfo.second;\n> -\n> -\t\tif (info->request == request)\n> -\t\t\treturn info;\n> -\t}\n> -\n> -\tLOG(RkISP1, Fatal) << \"Can't locate info from request\";\n> -\n> -\treturn nullptr;\n> -}\n> -\n> PipelineHandlerRkISP1 *RkISP1CameraData::pipe()\n> {\n> \treturn static_cast(Camera::Private::pipe());\n> @@ -391,23 +259,34 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> \treturn 0;\n> }\n> \n> +void RkISP1CameraData::addRequest(RkISP1Request *request)\n> +{\n> +\t/* Associate the request and the frame number. */\n> +\trequest->frame = frame_;\n> +\trequestMap_[frame_] = request;\n> +\tframe_++;\n> +}\n> +\n> void RkISP1CameraData::paramFilled(unsigned int frame)\n> {\n> \tPipelineHandlerRkISP1 *pipe = RkISP1CameraData::pipe();\n> -\tRkISP1FrameInfo *info = frameInfo_.find(frame);\n> -\tif (!info)\n> -\t\treturn;\n> +\tRkISP1Request *request = requestMap_.at(frame);\n> +\tASSERT(request);\n> \n> -\tinfo->paramBuffer->_d()->metadata().planes()[0].bytesused =\n> +\trequest->paramBuffer->_d()->metadata().planes()[0].bytesused =\n> \t\tsizeof(struct rkisp1_params_cfg);\n> -\tpipe->param_->queueBuffer(info->paramBuffer);\n> -\tpipe->stat_->queueBuffer(info->statBuffer);\n> -\n> -\tif (info->mainPathBuffer)\n> -\t\tmainPath_->queueBuffer(info->mainPathBuffer);\n> -\n> -\tif (selfPath_ && info->selfPathBuffer)\n> -\t\tselfPath_->queueBuffer(info->selfPathBuffer);\n> +\tpipe->param_->queueBuffer(request->paramBuffer);\n> +\tpipe->stat_->queueBuffer(request->statBuffer);\n> +\n> +\tFrameBuffer *mainPathBuffer =\n> +\t\trequest->_o()->findBuffer(&mainPathStream_);\n> +\tif (mainPathBuffer)\n> +\t\tmainPath_->queueBuffer(mainPathBuffer);\n> +\n> +\tFrameBuffer *selfPathBuffer =\n> +\t\trequest->_o()->findBuffer(&selfPathStream_);\n> +\tif (selfPath_ && selfPathBuffer)\n> +\t\tselfPath_->queueBuffer(selfPathBuffer);\n> }\n> \n> void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,\n> @@ -418,14 +297,13 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,\n> \n> void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n> {\n> -\tRkISP1FrameInfo *info = frameInfo_.find(frame);\n> -\tif (!info)\n> -\t\treturn;\n> +\tRkISP1Request *request = requestMap_.at(frame);\n> +\tASSERT(request);\n> \n> -\tinfo->request->metadata().merge(metadata);\n> -\tinfo->metadataProcessed = true;\n> +\trequest->_o()->metadata().merge(metadata);\n> +\trequest->metadataProcessed = true;\n> \n> -\tpipe()->tryCompleteRequest(info);\n> +\tpipe()->tryCompleteRequest(request);\n> }\n> \n> /* -----------------------------------------------------------------------------\n> @@ -1025,7 +903,7 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n> \t}\n> \n> \tASSERT(data->queuedRequests_.empty());\n> -\tdata->frameInfo_.clear();\n> +\tdata->requestMap_.clear();\n> \n> \tfreeBuffers(camera);\n> \n> @@ -1042,24 +920,60 @@ std::unique_ptr PipelineHandlerRkISP1::createRequestDevice(Camera *came\n> int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> {\n> \tRkISP1CameraData *data = cameraData(camera);\n> +\tRkISP1Request *rkisp1Request = cameraRequest(request);\n> \n> -\tRkISP1FrameInfo *info = data->frameInfo_.create(data, request, isRaw_);\n> -\tif (!info)\n> -\t\treturn -ENOENT;\n> +\tdata->addRequest(rkisp1Request);\n> \n> -\tdata->ipa_->queueRequest(data->frame_, request->controls());\n> +\t/*\n> +\t * If we're operating in RAW mode (only one RAW stream is captured)\n> +\t * then we simply queue buffers to the video devices as we don't\n> +\t * need to run the IPA.\n> +\t */\n> \tif (isRaw_) {\n> -\t\tif (info->mainPathBuffer)\n> -\t\t\tdata->mainPath_->queueBuffer(info->mainPathBuffer);\n> +\t\tFrameBuffer *mainPathBuffer =\n> +\t\t\trequest->findBuffer(&data->mainPathStream_);\n> +\t\tif (mainPathBuffer)\n> +\t\t\tdata->mainPath_->queueBuffer(mainPathBuffer);\n> \n> -\t\tif (data->selfPath_ && info->selfPathBuffer)\n> -\t\t\tdata->selfPath_->queueBuffer(info->selfPathBuffer);\n> -\t} else {\n> -\t\tdata->ipa_->fillParamsBuffer(data->frame_,\n> -\t\t\t\t\t info->paramBuffer->cookie());\n> +\t\tFrameBuffer *selfPathBuffer =\n> +\t\t\trequest->findBuffer(&data->selfPathStream_);\n> +\t\tif (data->selfPath_ && selfPathBuffer)\n> +\t\t\tdata->selfPath_->queueBuffer(selfPathBuffer);\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\t/*\n> +\t * If we run the IPA we need to associate a parameters and a statistics\n> +\t * buffer with the Request and associate the request with the current\n> +\t * frame number.\n> +\t *\n> +\t * Associate the stat and frame buffers to a Request (if available)\n> +\t * and then run the IPA.\n> +\t */\n> +\tif (availableParamBuffers_.empty()) {\n> +\t\tLOG(RkISP1, Error) << \"Parameters buffer underrun\";\n> +\t\treturn -ENOENT;\n> \t}\n> \n> -\tdata->frame_++;\n> +\tif (availableStatBuffers_.empty()) {\n> +\t\tLOG(RkISP1, Error) << \"Statistic buffer underrun\";\n> +\t\treturn -ENOENT;\n> +\t}\n> +\n> +\trkisp1Request->paramBuffer = availableParamBuffers_.front();\n> +\trkisp1Request->paramDequeued = false;\n> +\trkisp1Request->paramBuffer->_d()->setRequest(request);\n> +\tavailableParamBuffers_.pop();\n> +\n> +\trkisp1Request->statBuffer = availableStatBuffers_.front();\n> +\trkisp1Request->metadataProcessed = false;\n> +\trkisp1Request->statBuffer->_d()->setRequest(request);\n> +\tavailableStatBuffers_.pop();\n> +\n> +\tdata->ipa_->queueRequest(rkisp1Request->frame, request->controls());\n> +\tdata->ipa_->fillParamsBuffer(rkisp1Request->frame,\n> +\t\t\t\t rkisp1Request->paramBuffer->cookie());\n> \n> \treturn 0;\n> }\n> @@ -1251,37 +1165,28 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> * Buffer Handling\n> */\n> \n> -void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)\n> +void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1Request *request)\n> {\n> \tRkISP1CameraData *data = cameraData(activeCamera_);\n> -\tRequest *request = info->request;\n> -\n> -\tif (request->hasPendingBuffers())\n> -\t\treturn;\n> -\n> -\tif (!info->metadataProcessed)\n> -\t\treturn;\n> \n> -\tif (!isRaw_ && !info->paramDequeued)\n> +\tif (request->hasPendingBuffers(isRaw_))\n> \t\treturn;\n> \n> -\tdata->frameInfo_.destroy(info->frame);\n> +\t/* Return the stat and param buffers to the pipeline. */\n> +\tavailableParamBuffers_.push(request->paramBuffer);\n> +\tavailableStatBuffers_.push(request->statBuffer);\n> +\tdata->requestMap_.erase(request->frame);\n> \n> -\tcompleteRequest(request);\n> +\tcompleteRequest(request->_o());\n> }\n> \n> void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> {\n> \tASSERT(activeCamera_);\n> \tRkISP1CameraData *data = cameraData(activeCamera_);\n> -\n> -\tRkISP1FrameInfo *info = data->frameInfo_.find(buffer);\n> -\tif (!info)\n> -\t\treturn;\n> +\tRkISP1Request *request = cameraRequest(buffer->request());\n> \n> \tconst FrameMetadata &metadata = buffer->metadata();\n> -\tRequest *request = buffer->request();\n> -\n> \tif (metadata.status != FrameMetadata::FrameCancelled) {\n> \t\t/*\n> \t\t * Record the sensor's timestamp in the request metadata.\n> @@ -1289,55 +1194,48 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> \t\t * \\todo The sensor timestamp should be better estimated by connecting\n> \t\t * to the V4L2Device::frameStart signal.\n> \t\t */\n> -\t\trequest->metadata().set(controls::SensorTimestamp,\n> -\t\t\t\t\tmetadata.timestamp);\n> +\t\trequest->_o()->metadata().set(controls::SensorTimestamp,\n> +\t\t\t\t\t\t metadata.timestamp);\n> \n> \t\tif (isRaw_) {\n> \t\t\tconst ControlList &ctrls =\n> \t\t\t\tdata->delayedCtrls_->get(metadata.sequence);\n> -\t\t\tdata->ipa_->processStatsBuffer(info->frame, 0, ctrls);\n> +\t\t\tdata->ipa_->processStatsBuffer(request->frame, 0, ctrls);\n> \t\t}\n> \t} else {\n> \t\tif (isRaw_)\n> -\t\t\tinfo->metadataProcessed = true;\n> +\t\t\trequest->metadataProcessed = true;\n> \t}\n> \n> -\tcompleteBuffer(request, buffer);\n> -\ttryCompleteRequest(info);\n> +\tcompleteBuffer(request->_o(), buffer);\n> +\ttryCompleteRequest(request);\n> }\n> \n> void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)\n> {\n> \tASSERT(activeCamera_);\n> -\tRkISP1CameraData *data = cameraData(activeCamera_);\n> +\tRkISP1Request *request = cameraRequest(buffer->request());\n> \n> -\tRkISP1FrameInfo *info = data->frameInfo_.find(buffer);\n> -\tif (!info)\n> -\t\treturn;\n> -\n> -\tinfo->paramDequeued = true;\n> -\ttryCompleteRequest(info);\n> +\trequest->paramDequeued = true;\n> +\ttryCompleteRequest(request);\n> }\n> \n> void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> {\n> \tASSERT(activeCamera_);\n> \tRkISP1CameraData *data = cameraData(activeCamera_);\n> -\n> -\tRkISP1FrameInfo *info = data->frameInfo_.find(buffer);\n> -\tif (!info)\n> -\t\treturn;\n> +\tRkISP1Request *request = cameraRequest(buffer->request());\n> \n> \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> -\t\tinfo->metadataProcessed = true;\n> -\t\ttryCompleteRequest(info);\n> +\t\trequest->metadataProcessed = true;\n> +\t\ttryCompleteRequest(request);\n> \t\treturn;\n> \t}\n> \n> \tif (data->frame_ <= buffer->metadata().sequence)\n> \t\tdata->frame_ = buffer->metadata().sequence + 1;\n> \n> -\tdata->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),\n> +\tdata->ipa_->processStatsBuffer(request->frame, request->statBuffer->cookie(),\n> \t\t\t\t data->delayedCtrls_->get(buffer->metadata().sequence));\n> }\n> \n> -- \n> 2.43.0\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 DE610C0F2A\n\tfor ;\n\tWed, 6 Mar 2024 15:48:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 30B6F62872;\n\tWed, 6 Mar 2024 16:48:19 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CA66061C8F\n\tfor ;\n\tWed, 6 Mar 2024 16:48:17 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:dd65:a3ea:5f4d:989a])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F023B552;\n\tWed, 6 Mar 2024 16:47:59 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"WhfRmSMS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709740080;\n\tbh=+G6sT8oNj8KmbN/HbdmXIXSjoj8iirFtmV1LqsRhhgM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WhfRmSMSYOxJd88M4lwtbzs0vuQQIPr87sfSo5yDbnbDPhhNxTyd/wGSvKmA1emY6\n\tLHjBJkXn0KEgoFqnh3sD7ksu95jfuNKRAe0zgKQV+XKv4Vb3yP25ULts5+t7s56ygg\n\txi+8/C36O0GE4NPaPGZ26Mggop/AS7Ad83xO5zRw=","Date":"Wed, 6 Mar 2024 16:48:14 +0100","From":"Stefan Klug ","To":"Jacopo Mondi ","Subject":"Re: [PATCH 3/5] libcamera: rkisp1: Replace usage of RkISP1FrameInfo","Message-ID":"<20240306154814.4w5ebb7rrjiqquru@jasper>","References":"<20240221174015.52958-1-jacopo.mondi@ideasonboard.com>\n\t<20240221174015.52958-4-jacopo.mondi@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240221174015.52958-4-jacopo.mondi@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28908,"web_url":"https://patchwork.libcamera.org/comment/28908/","msgid":"","date":"2024-03-11T11:40:34","subject":"Re: [PATCH 3/5] libcamera: rkisp1: Replace usage of RkISP1FrameInfo","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Dan\n\nOn Tue, Mar 05, 2024 at 11:50:11AM +0000, Dan Scally wrote:\n> Hi Jacopo\n>\n> On 21/02/2024 17:40, Jacopo Mondi wrote:\n> > Now that the pipeline handler can create a private derived class of\n> > Request::Private, use it to store the pipeline specific data.\n> >\n> > In the case of RkISP1 we associate the statistic and paramters buffers\n> > with a Request.\n> >\n> > As the IPA sends notifications for paramters and statistics buffer by\n> > identifying them by frame id, associate the frame ids and the Request\n> > in the RkISP1CameraData class.\n> >\n> > This replaces the functionalities of RkISP1FrameInfo which can now be\n> > removed.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n>\n>\n> Code here looks fine to me so:\n>\n> Reviewed-by: Daniel Scally \n>\n> and\n>\n> Tested-by: Daniel Scally \n>\n>\n> However I do wonder if the change in queueRequestDevice() to stop\n> unconditionally queuing frames to the IPA should go into its own commit\n> before this one - it's a good change but also could stand alone. Up to you\n> though.\n>\n\nGood catch!\n\nThe difference is that we used to queue Request to the IPA even for\nRAW frames, even if we didn't provide any paramters buffer to fill so\nnot ISP (or sensor) paramters where computed, but the algorithms were\nrun anyway.\n\nWith this new version we only queue a request to the IPA for non-RAW\nrequests.\n\nI would rather maintain the existing behaviour and hence change this\npatch in my next version\n\nThanks\n j\n\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 358 ++++++++---------------\n> > 1 file changed, 128 insertions(+), 230 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index e981e60758f7..f2163f528251 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -49,76 +49,63 @@ namespace libcamera {\n> > LOG_DEFINE_CATEGORY(RkISP1)\n> > -class PipelineHandlerRkISP1;\n> > -class RkISP1CameraData;\n> > +class RkISP1Request : public Request::Private\n> > +{\n> > +public:\n> > +\tRkISP1Request(Camera *camera)\n> > +\t\t: Request::Private(camera)\n> > +\t{\n> > +\t}\n> > -struct RkISP1FrameInfo {\n> > -\tunsigned int frame;\n> > -\tRequest *request;\n> > +\tbool hasPendingBuffers(bool isRaw) const;\n> > -\tFrameBuffer *paramBuffer;\n> > \tFrameBuffer *statBuffer;\n> > -\tFrameBuffer *mainPathBuffer;\n> > -\tFrameBuffer *selfPathBuffer;\n> > +\tFrameBuffer *paramBuffer;\n> > +\n> > +\t/* The frame number this request is associated with. */\n> > +\tunsigned int frame;\n> > \tbool paramDequeued;\n> > \tbool metadataProcessed;\n> > };\n> > -class RkISP1Frames\n> > -{\n> > -public:\n> > -\tRkISP1Frames(PipelineHandler *pipe);\n> > -\n> > -\tRkISP1FrameInfo *create(const RkISP1CameraData *data, Request *request,\n> > -\t\t\t\tbool isRaw);\n> > -\tint destroy(unsigned int frame);\n> > -\tvoid clear();\n> > -\n> > -\tRkISP1FrameInfo *find(unsigned int frame);\n> > -\tRkISP1FrameInfo *find(FrameBuffer *buffer);\n> > -\tRkISP1FrameInfo *find(Request *request);\n> > -\n> > -private:\n> > -\tPipelineHandlerRkISP1 *pipe_;\n> > -\tstd::map frameInfo_;\n> > -};\n> > -\n> > -class RkISP1Request : public Request::Private\n> > +bool RkISP1Request::hasPendingBuffers(bool isRaw) const\n> > {\n> > -public:\n> > -\tRkISP1Request(Camera *camera)\n> > -\t\t: Request::Private(camera)\n> > -\t{\n> > -\t}\n> > -};\n> > +\treturn Request::Private::hasPendingBuffers() ||\n> > +\t !metadataProcessed || (!isRaw && !paramDequeued);\n> > +}\n> > +class PipelineHandlerRkISP1;\n> > class RkISP1CameraData : public Camera::Private\n> > {\n> > public:\n> > \tRkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath,\n> > \t\t\t RkISP1SelfPath *selfPath)\n> > -\t\t: Camera::Private(pipe), frame_(0), frameInfo_(pipe),\n> > -\t\t mainPath_(mainPath), selfPath_(selfPath)\n> > +\t\t: Camera::Private(pipe), frame_(0), mainPath_(mainPath),\n> > +\t\t selfPath_(selfPath)\n> > \t{\n> > \t}\n> > \tPipelineHandlerRkISP1 *pipe();\n> > \tint loadIPA(unsigned int hwRevision);\n> > +\tvoid addRequest(RkISP1Request *request);\n> > +\n> > \tStream mainPathStream_;\n> > \tStream selfPathStream_;\n> > \tstd::unique_ptr sensor_;\n> > \tstd::unique_ptr delayedCtrls_;\n> > \tunsigned int frame_;\n> > \tstd::vector ipaBuffers_;\n> > -\tRkISP1Frames frameInfo_;\n> > \tRkISP1MainPath *mainPath_;\n> > \tRkISP1SelfPath *selfPath_;\n> > \tstd::unique_ptr ipa_;\n> > +\t/* Associate a frame id with a Request. */\n> > +\tstd::map requestMap_;\n> > +\n> > private:\n> > \tvoid paramFilled(unsigned int frame);\n> > \tvoid setSensorControls(unsigned int frame,\n> > @@ -181,13 +168,17 @@ private:\n> > \t\treturn static_cast(camera->_d());\n> > \t}\n> > +\tRkISP1Request *cameraRequest(Request *request)\n> > +\t{\n> > +\t\treturn static_cast(request->_d());\n> > +\t}\n> > +\n> > \tfriend RkISP1CameraData;\n> > -\tfriend RkISP1Frames;\n> > \tint initLinks(Camera *camera, const CameraSensor *sensor,\n> > \t\t const RkISP1CameraConfiguration &config);\n> > \tint createCamera(MediaEntity *sensor);\n> > -\tvoid tryCompleteRequest(RkISP1FrameInfo *info);\n> > +\tvoid tryCompleteRequest(RkISP1Request *request);\n> > \tvoid bufferReady(FrameBuffer *buffer);\n> > \tvoid paramReady(FrameBuffer *buffer);\n> > \tvoid statReady(FrameBuffer *buffer);\n> > @@ -218,129 +209,6 @@ private:\n> > \tconst MediaPad *ispSink_;\n> > };\n> > -RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)\n> > -\t: pipe_(static_cast(pipe))\n> > -{\n> > -}\n> > -\n> > -RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *request,\n> > -\t\t\t\t bool isRaw)\n> > -{\n> > -\tunsigned int frame = data->frame_;\n> > -\n> > -\tFrameBuffer *paramBuffer = nullptr;\n> > -\tFrameBuffer *statBuffer = nullptr;\n> > -\n> > -\tif (!isRaw) {\n> > -\t\tif (pipe_->availableParamBuffers_.empty()) {\n> > -\t\t\tLOG(RkISP1, Error) << \"Parameters buffer underrun\";\n> > -\t\t\treturn nullptr;\n> > -\t\t}\n> > -\n> > -\t\tif (pipe_->availableStatBuffers_.empty()) {\n> > -\t\t\tLOG(RkISP1, Error) << \"Statistic buffer underrun\";\n> > -\t\t\treturn nullptr;\n> > -\t\t}\n> > -\n> > -\t\tparamBuffer = pipe_->availableParamBuffers_.front();\n> > -\t\tpipe_->availableParamBuffers_.pop();\n> > -\n> > -\t\tstatBuffer = pipe_->availableStatBuffers_.front();\n> > -\t\tpipe_->availableStatBuffers_.pop();\n> > -\t}\n> > -\n> > -\tFrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> > -\tFrameBuffer *selfPathBuffer = request->findBuffer(&data->selfPathStream_);\n> > -\n> > -\tRkISP1FrameInfo *info = new RkISP1FrameInfo;\n> > -\n> > -\tinfo->frame = frame;\n> > -\tinfo->request = request;\n> > -\tinfo->paramBuffer = paramBuffer;\n> > -\tinfo->mainPathBuffer = mainPathBuffer;\n> > -\tinfo->selfPathBuffer = selfPathBuffer;\n> > -\tinfo->statBuffer = statBuffer;\n> > -\tinfo->paramDequeued = false;\n> > -\tinfo->metadataProcessed = false;\n> > -\n> > -\tframeInfo_[frame] = info;\n> > -\n> > -\treturn info;\n> > -}\n> > -\n> > -int RkISP1Frames::destroy(unsigned int frame)\n> > -{\n> > -\tRkISP1FrameInfo *info = find(frame);\n> > -\tif (!info)\n> > -\t\treturn -ENOENT;\n> > -\n> > -\tpipe_->availableParamBuffers_.push(info->paramBuffer);\n> > -\tpipe_->availableStatBuffers_.push(info->statBuffer);\n> > -\n> > -\tframeInfo_.erase(info->frame);\n> > -\n> > -\tdelete info;\n> > -\n> > -\treturn 0;\n> > -}\n> > -\n> > -void RkISP1Frames::clear()\n> > -{\n> > -\tfor (const auto &entry : frameInfo_) {\n> > -\t\tRkISP1FrameInfo *info = entry.second;\n> > -\n> > -\t\tpipe_->availableParamBuffers_.push(info->paramBuffer);\n> > -\t\tpipe_->availableStatBuffers_.push(info->statBuffer);\n> > -\n> > -\t\tdelete info;\n> > -\t}\n> > -\n> > -\tframeInfo_.clear();\n> > -}\n> > -\n> > -RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)\n> > -{\n> > -\tauto itInfo = frameInfo_.find(frame);\n> > -\n> > -\tif (itInfo != frameInfo_.end())\n> > -\t\treturn itInfo->second;\n> > -\n> > -\tLOG(RkISP1, Fatal) << \"Can't locate info from frame\";\n> > -\n> > -\treturn nullptr;\n> > -}\n> > -\n> > -RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n> > -{\n> > -\tfor (auto &itInfo : frameInfo_) {\n> > -\t\tRkISP1FrameInfo *info = itInfo.second;\n> > -\n> > -\t\tif (info->paramBuffer == buffer ||\n> > -\t\t info->statBuffer == buffer ||\n> > -\t\t info->mainPathBuffer == buffer ||\n> > -\t\t info->selfPathBuffer == buffer)\n> > -\t\t\treturn info;\n> > -\t}\n> > -\n> > -\tLOG(RkISP1, Fatal) << \"Can't locate info from buffer\";\n> > -\n> > -\treturn nullptr;\n> > -}\n> > -\n> > -RkISP1FrameInfo *RkISP1Frames::find(Request *request)\n> > -{\n> > -\tfor (auto &itInfo : frameInfo_) {\n> > -\t\tRkISP1FrameInfo *info = itInfo.second;\n> > -\n> > -\t\tif (info->request == request)\n> > -\t\t\treturn info;\n> > -\t}\n> > -\n> > -\tLOG(RkISP1, Fatal) << \"Can't locate info from request\";\n> > -\n> > -\treturn nullptr;\n> > -}\n> > -\n> > PipelineHandlerRkISP1 *RkISP1CameraData::pipe()\n> > {\n> > \treturn static_cast(Camera::Private::pipe());\n> > @@ -391,23 +259,34 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > \treturn 0;\n> > }\n> > +void RkISP1CameraData::addRequest(RkISP1Request *request)\n> > +{\n> > +\t/* Associate the request and the frame number. */\n> > +\trequest->frame = frame_;\n> > +\trequestMap_[frame_] = request;\n> > +\tframe_++;\n> > +}\n> > +\n> > void RkISP1CameraData::paramFilled(unsigned int frame)\n> > {\n> > \tPipelineHandlerRkISP1 *pipe = RkISP1CameraData::pipe();\n> > -\tRkISP1FrameInfo *info = frameInfo_.find(frame);\n> > -\tif (!info)\n> > -\t\treturn;\n> > +\tRkISP1Request *request = requestMap_.at(frame);\n> > +\tASSERT(request);\n> > -\tinfo->paramBuffer->_d()->metadata().planes()[0].bytesused =\n> > +\trequest->paramBuffer->_d()->metadata().planes()[0].bytesused =\n> > \t\tsizeof(struct rkisp1_params_cfg);\n> > -\tpipe->param_->queueBuffer(info->paramBuffer);\n> > -\tpipe->stat_->queueBuffer(info->statBuffer);\n> > -\n> > -\tif (info->mainPathBuffer)\n> > -\t\tmainPath_->queueBuffer(info->mainPathBuffer);\n> > -\n> > -\tif (selfPath_ && info->selfPathBuffer)\n> > -\t\tselfPath_->queueBuffer(info->selfPathBuffer);\n> > +\tpipe->param_->queueBuffer(request->paramBuffer);\n> > +\tpipe->stat_->queueBuffer(request->statBuffer);\n> > +\n> > +\tFrameBuffer *mainPathBuffer =\n> > +\t\trequest->_o()->findBuffer(&mainPathStream_);\n> > +\tif (mainPathBuffer)\n> > +\t\tmainPath_->queueBuffer(mainPathBuffer);\n> > +\n> > +\tFrameBuffer *selfPathBuffer =\n> > +\t\trequest->_o()->findBuffer(&selfPathStream_);\n> > +\tif (selfPath_ && selfPathBuffer)\n> > +\t\tselfPath_->queueBuffer(selfPathBuffer);\n> > }\n> > void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,\n> > @@ -418,14 +297,13 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,\n> > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n> > {\n> > -\tRkISP1FrameInfo *info = frameInfo_.find(frame);\n> > -\tif (!info)\n> > -\t\treturn;\n> > +\tRkISP1Request *request = requestMap_.at(frame);\n> > +\tASSERT(request);\n> > -\tinfo->request->metadata().merge(metadata);\n> > -\tinfo->metadataProcessed = true;\n> > +\trequest->_o()->metadata().merge(metadata);\n> > +\trequest->metadataProcessed = true;\n> > -\tpipe()->tryCompleteRequest(info);\n> > +\tpipe()->tryCompleteRequest(request);\n> > }\n> > /* -----------------------------------------------------------------------------\n> > @@ -1025,7 +903,7 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n> > \t}\n> > \tASSERT(data->queuedRequests_.empty());\n> > -\tdata->frameInfo_.clear();\n> > +\tdata->requestMap_.clear();\n> > \tfreeBuffers(camera);\n> > @@ -1042,24 +920,60 @@ std::unique_ptr PipelineHandlerRkISP1::createRequestDevice(Camera *came\n> > int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> > {\n> > \tRkISP1CameraData *data = cameraData(camera);\n> > +\tRkISP1Request *rkisp1Request = cameraRequest(request);\n> > -\tRkISP1FrameInfo *info = data->frameInfo_.create(data, request, isRaw_);\n> > -\tif (!info)\n> > -\t\treturn -ENOENT;\n> > +\tdata->addRequest(rkisp1Request);\n> > -\tdata->ipa_->queueRequest(data->frame_, request->controls());\n> > +\t/*\n> > +\t * If we're operating in RAW mode (only one RAW stream is captured)\n> > +\t * then we simply queue buffers to the video devices as we don't\n> > +\t * need to run the IPA.\n> > +\t */\n> > \tif (isRaw_) {\n> > -\t\tif (info->mainPathBuffer)\n> > -\t\t\tdata->mainPath_->queueBuffer(info->mainPathBuffer);\n> > +\t\tFrameBuffer *mainPathBuffer =\n> > +\t\t\trequest->findBuffer(&data->mainPathStream_);\n> > +\t\tif (mainPathBuffer)\n> > +\t\t\tdata->mainPath_->queueBuffer(mainPathBuffer);\n> > -\t\tif (data->selfPath_ && info->selfPathBuffer)\n> > -\t\t\tdata->selfPath_->queueBuffer(info->selfPathBuffer);\n> > -\t} else {\n> > -\t\tdata->ipa_->fillParamsBuffer(data->frame_,\n> > -\t\t\t\t\t info->paramBuffer->cookie());\n> > +\t\tFrameBuffer *selfPathBuffer =\n> > +\t\t\trequest->findBuffer(&data->selfPathStream_);\n> > +\t\tif (data->selfPath_ && selfPathBuffer)\n> > +\t\t\tdata->selfPath_->queueBuffer(selfPathBuffer);\n> > +\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * If we run the IPA we need to associate a parameters and a statistics\n> > +\t * buffer with the Request and associate the request with the current\n> > +\t * frame number.\n> > +\t *\n> > +\t * Associate the stat and frame buffers to a Request (if available)\n> > +\t * and then run the IPA.\n> > +\t */\n> > +\tif (availableParamBuffers_.empty()) {\n> > +\t\tLOG(RkISP1, Error) << \"Parameters buffer underrun\";\n> > +\t\treturn -ENOENT;\n> > \t}\n> > -\tdata->frame_++;\n> > +\tif (availableStatBuffers_.empty()) {\n> > +\t\tLOG(RkISP1, Error) << \"Statistic buffer underrun\";\n> > +\t\treturn -ENOENT;\n> > +\t}\n> > +\n> > +\trkisp1Request->paramBuffer = availableParamBuffers_.front();\n> > +\trkisp1Request->paramDequeued = false;\n> > +\trkisp1Request->paramBuffer->_d()->setRequest(request);\n> > +\tavailableParamBuffers_.pop();\n> > +\n> > +\trkisp1Request->statBuffer = availableStatBuffers_.front();\n> > +\trkisp1Request->metadataProcessed = false;\n> > +\trkisp1Request->statBuffer->_d()->setRequest(request);\n> > +\tavailableStatBuffers_.pop();\n> > +\n> > +\tdata->ipa_->queueRequest(rkisp1Request->frame, request->controls());\n> > +\tdata->ipa_->fillParamsBuffer(rkisp1Request->frame,\n> > +\t\t\t\t rkisp1Request->paramBuffer->cookie());\n> > \treturn 0;\n> > }\n> > @@ -1251,37 +1165,28 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > * Buffer Handling\n> > */\n> > -void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)\n> > +void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1Request *request)\n> > {\n> > \tRkISP1CameraData *data = cameraData(activeCamera_);\n> > -\tRequest *request = info->request;\n> > -\n> > -\tif (request->hasPendingBuffers())\n> > -\t\treturn;\n> > -\n> > -\tif (!info->metadataProcessed)\n> > -\t\treturn;\n> > -\tif (!isRaw_ && !info->paramDequeued)\n> > +\tif (request->hasPendingBuffers(isRaw_))\n> > \t\treturn;\n> > -\tdata->frameInfo_.destroy(info->frame);\n> > +\t/* Return the stat and param buffers to the pipeline. */\n> > +\tavailableParamBuffers_.push(request->paramBuffer);\n> > +\tavailableStatBuffers_.push(request->statBuffer);\n> > +\tdata->requestMap_.erase(request->frame);\n> > -\tcompleteRequest(request);\n> > +\tcompleteRequest(request->_o());\n> > }\n> > void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> > {\n> > \tASSERT(activeCamera_);\n> > \tRkISP1CameraData *data = cameraData(activeCamera_);\n> > -\n> > -\tRkISP1FrameInfo *info = data->frameInfo_.find(buffer);\n> > -\tif (!info)\n> > -\t\treturn;\n> > +\tRkISP1Request *request = cameraRequest(buffer->request());\n> > \tconst FrameMetadata &metadata = buffer->metadata();\n> > -\tRequest *request = buffer->request();\n> > -\n> > \tif (metadata.status != FrameMetadata::FrameCancelled) {\n> > \t\t/*\n> > \t\t * Record the sensor's timestamp in the request metadata.\n> > @@ -1289,55 +1194,48 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> > \t\t * \\todo The sensor timestamp should be better estimated by connecting\n> > \t\t * to the V4L2Device::frameStart signal.\n> > \t\t */\n> > -\t\trequest->metadata().set(controls::SensorTimestamp,\n> > -\t\t\t\t\tmetadata.timestamp);\n> > +\t\trequest->_o()->metadata().set(controls::SensorTimestamp,\n> > +\t\t\t\t\t\t metadata.timestamp);\n> > \t\tif (isRaw_) {\n> > \t\t\tconst ControlList &ctrls =\n> > \t\t\t\tdata->delayedCtrls_->get(metadata.sequence);\n> > -\t\t\tdata->ipa_->processStatsBuffer(info->frame, 0, ctrls);\n> > +\t\t\tdata->ipa_->processStatsBuffer(request->frame, 0, ctrls);\n> > \t\t}\n> > \t} else {\n> > \t\tif (isRaw_)\n> > -\t\t\tinfo->metadataProcessed = true;\n> > +\t\t\trequest->metadataProcessed = true;\n> > \t}\n> > -\tcompleteBuffer(request, buffer);\n> > -\ttryCompleteRequest(info);\n> > +\tcompleteBuffer(request->_o(), buffer);\n> > +\ttryCompleteRequest(request);\n> > }\n> > void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)\n> > {\n> > \tASSERT(activeCamera_);\n> > -\tRkISP1CameraData *data = cameraData(activeCamera_);\n> > +\tRkISP1Request *request = cameraRequest(buffer->request());\n> > -\tRkISP1FrameInfo *info = data->frameInfo_.find(buffer);\n> > -\tif (!info)\n> > -\t\treturn;\n> > -\n> > -\tinfo->paramDequeued = true;\n> > -\ttryCompleteRequest(info);\n> > +\trequest->paramDequeued = true;\n> > +\ttryCompleteRequest(request);\n> > }\n> > void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> > {\n> > \tASSERT(activeCamera_);\n> > \tRkISP1CameraData *data = cameraData(activeCamera_);\n> > -\n> > -\tRkISP1FrameInfo *info = data->frameInfo_.find(buffer);\n> > -\tif (!info)\n> > -\t\treturn;\n> > +\tRkISP1Request *request = cameraRequest(buffer->request());\n> > \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > -\t\tinfo->metadataProcessed = true;\n> > -\t\ttryCompleteRequest(info);\n> > +\t\trequest->metadataProcessed = true;\n> > +\t\ttryCompleteRequest(request);\n> > \t\treturn;\n> > \t}\n> > \tif (data->frame_ <= buffer->metadata().sequence)\n> > \t\tdata->frame_ = buffer->metadata().sequence + 1;\n> > -\tdata->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),\n> > +\tdata->ipa_->processStatsBuffer(request->frame, request->statBuffer->cookie(),\n> > \t\t\t\t data->delayedCtrls_->get(buffer->metadata().sequence));\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 2B0FEBD1F1\n\tfor ;\n\tMon, 11 Mar 2024 11:40:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2ADBC62C80;\n\tMon, 11 Mar 2024 12:40:39 +0100 (CET)","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 3B41661C7C\n\tfor ;\n\tMon, 11 Mar 2024 12:40:37 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0CA2BE4;\n\tMon, 11 Mar 2024 12:40:16 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"j6H8Mum0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710157216;\n\tbh=JGPcEsCkA74xh8GCxgWwd9ynSsYmIjrqB8MnSZv8fuk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=j6H8Mum0VOotYlBITp9CjsmpBNp3CyyuyWwmrluNJPk2Q4suy96D56JzU3CKGv1eB\n\tuOiAMSfV4VhEpLnC1eJOGxNyzdNoqoAVkMtbh1EKqqlhMwpB1gQNmbyBScJSXfQuSI\n\tacckgpOEJqnRUQDG9QKoHzKSqRUXNJs1f+ZKwX5E=","Date":"Mon, 11 Mar 2024 12:40:34 +0100","From":"Jacopo Mondi ","To":"Dan Scally ","Subject":"Re: [PATCH 3/5] libcamera: rkisp1: Replace usage of RkISP1FrameInfo","Message-ID":"","References":"<20240221174015.52958-1-jacopo.mondi@ideasonboard.com>\n\t<20240221174015.52958-4-jacopo.mondi@ideasonboard.com>\n\t<21e0c6c6-35bd-42ec-86fe-480285082cdf@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<21e0c6c6-35bd-42ec-86fe-480285082cdf@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]