[{"id":31671,"web_url":"https://patchwork.libcamera.org/comment/31671/","msgid":"<172850912219.532453.3576191739872514782@ping.linuxembedded.co.uk>","date":"2024-10-09T21:25:22","subject":"Re: [PATCH v2 05/10] mali-c55: Plumb the IPA module in","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Daniel Scally (2024-07-09 15:49:45)\n> From: Jacopo Mondi \n> \n> Plumb the Pipeline-IPA loop in.\n> \n> Load the IPA module at camera creation time and create the loop between\n> the pipeline and the IPA.\n> \n> When a new Request is queued the IPA is asked to prepare the parameters\n> buffer, once ready it notifies the pipeline which queues the parameters\n> to the ISP along with a buffer for statistics and frames,\n> \n> Once statistics are ready they get passed to the IPA which upates its\n> settings for the next frame.\n> \n> Acked-by: Nayden Kanchev \n> Signed-off-by: Jacopo Mondi \n> ---\n> Changes in v2:\n> \n> - None\n> \n> src/libcamera/pipeline/mali-c55/mali-c55.cpp | 403 +++++++++++++++++--\n> 1 file changed, 360 insertions(+), 43 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> index 1e5674fc..dd523d8d 100644\n> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> @@ -23,13 +23,19 @@\n> #include \n> #include \n> \n> +#include \n> +#include \n> +\n> #include \"libcamera/internal/bayer_format.h\"\n> #include \"libcamera/internal/camera.h\"\n> #include \"libcamera/internal/camera_sensor.h\"\n> +#include \"libcamera/internal/delayed_controls.h\"\n> #include \"libcamera/internal/device_enumerator.h\"\n> #include \"libcamera/internal/framebuffer.h\"\n> +#include \"libcamera/internal/ipa_manager.h\"\n> #include \"libcamera/internal/media_device.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n> +#include \"libcamera/internal/request.h\"\n> #include \"libcamera/internal/v4l2_subdevice.h\"\n> #include \"libcamera/internal/v4l2_videodevice.h\"\n> \n> @@ -70,6 +76,16 @@ constexpr Size kMaliC55MinSize = { 128, 128 };\n> constexpr Size kMaliC55MaxSize = { 8192, 8192 };\n> constexpr unsigned int kMaliC55ISPInternalFormat = MEDIA_BUS_FMT_RGB121212_1X36;\n> \n> +struct MaliC55FrameInfo {\n> + Request *request;\n> +\n> + FrameBuffer *paramBuffer;\n> + FrameBuffer *statBuffer;\n> +\n> + bool paramsDone;\n> + bool statsDone;\n> +};\n> +\n> class MaliC55CameraData : public Camera::Private\n> {\n> public:\n> @@ -79,6 +95,7 @@ public:\n> }\n> \n> int init();\n> + int loadIPA();\n> \n> /* Deflect these functionalities to either TPG or CameraSensor. */\n> const std::vector sizes(unsigned int mbusCode) const;\n> @@ -87,7 +104,7 @@ public:\n> int pixfmtToMbusCode(const PixelFormat &pixFmt) const;\n> const PixelFormat &bestRawFormat() const;\n> \n> - void updateControls();\n> + void updateControls(const ControlInfoMap &ipaControls);\n> \n> PixelFormat adjustRawFormat(const PixelFormat &pixFmt) const;\n> Size adjustRawSizes(const PixelFormat &pixFmt, const Size &rawSize) const;\n> @@ -100,8 +117,15 @@ public:\n> Stream frStream_;\n> Stream dsStream_;\n> \n> + std::unique_ptr ipa_;\n> + std::vector ipaStatBuffers_;\n> + std::vector ipaParamBuffers_;\n> +\n> + std::unique_ptr delayedCtrls_;\n> +\n> private:\n> void initTPGData();\n> + void setSensorControls(const ControlList &sensorControls);\n> \n> std::string id_;\n> std::vector tpgCodes_;\n> @@ -166,6 +190,11 @@ void MaliC55CameraData::initTPGData()\n> tpgResolution_ = tpgSizes_.back();\n> }\n> \n> +void MaliC55CameraData::setSensorControls(const ControlList &sensorControls)\n> +{\n> + delayedCtrls_->push(sensorControls);\n> +}\n> +\n> const std::vector MaliC55CameraData::sizes(unsigned int mbusCode) const\n> {\n> if (sensor_)\n> @@ -268,7 +297,7 @@ const PixelFormat &MaliC55CameraData::bestRawFormat() const\n> return invalidPixFmt;\n> }\n> \n> -void MaliC55CameraData::updateControls()\n> +void MaliC55CameraData::updateControls(const ControlInfoMap &ipaControls)\n> {\n> if (!sensor_)\n> return;\n> @@ -286,6 +315,9 @@ void MaliC55CameraData::updateControls()\n> ControlInfo(ispMinCrop, sensorInfo.analogCrop,\n> sensorInfo.analogCrop);\n> \n> + for (auto const &c : ipaControls)\n> + controls.emplace(c.first, c.second);\n> +\n> controlInfo_ = ControlInfoMap(std::move(controls), controls::controls);\n> }\n> \n> @@ -339,6 +371,46 @@ Size MaliC55CameraData::adjustRawSizes(const PixelFormat &rawFmt, const Size &si\n> return bestSize;\n> }\n> \n> +int MaliC55CameraData::loadIPA()\n> +{\n> + int ret;\n> +\n> + /* Do not initialize IPA for TPG. */\n> + if (!sensor_)\n> + return 0;\n> +\n> + ipa_ = IPAManager::createIPA(pipe(), 1, 1);\n> + if (!ipa_)\n> + return -ENOENT;\n> +\n> + ipa_->setSensorControls.connect(this, &MaliC55CameraData::setSensorControls);\n> +\n> + std::string ipaTuningFile = ipa_->configurationFile(sensor_->model() + \".yaml\");\n> + if (ipaTuningFile.empty())\n> + ipaTuningFile = ipa_->configurationFile(\"uncalibrated.yaml\");\n\nI think this (ipa_->configurationFile) has since been modified to take\nthe fallback file path. So this probably needs to be updated with a\nrebase to \n\n\tipa_->configurationFile(sensor->model() + \".yaml\", \"uncalibrated.yaml\");\n\n\n> +\n> + /* We need to inform the IPA of the sensor configuration */\n> + ipa::mali_c55::IPAConfigInfo ipaConfig{};\n> +\n> + ret = sensor_->sensorInfo(&ipaConfig.sensorInfo);\n> + if (ret)\n> + return ret;\n> +\n> + ipaConfig.sensorControls = sensor_->controls();\n> +\n> + ControlInfoMap ipaControls;\n> + ret = ipa_->init({ ipaTuningFile, sensor_->model() }, ipaConfig,\n> + &ipaControls);\n> + if (ret) {\n> + LOG(MaliC55, Error) << \"Failed to initialise the Mali-C55 IPA\";\n> + return ret;\n> + }\n> +\n> + updateControls(ipaControls);\n> +\n> + return 0;\n> +}\n> +\n> class MaliC55CameraConfiguration : public CameraConfiguration\n> {\n> public:\n> @@ -348,6 +420,7 @@ public:\n> }\n> \n> Status validate() override;\n> + const Transform &combinedTransform() { return combinedTransform_; }\n> \n> V4L2SubdeviceFormat sensorFormat_;\n> \n> @@ -355,6 +428,7 @@ private:\n> static constexpr unsigned int kMaxStreams = 2;\n> \n> const MaliC55CameraData *data_;\n> + Transform combinedTransform_;\n> };\n> \n> CameraConfiguration::Status MaliC55CameraConfiguration::validate()\n> @@ -364,6 +438,19 @@ CameraConfiguration::Status MaliC55CameraConfiguration::validate()\n> if (config_.empty())\n> return Invalid;\n> \n> + /*\n> + * The TPG doesn't support flips, so we only need to calculate a\n> + * transform if we have a sensor.\n> + */\n> + if (data_->sensor_) {\n> + Orientation requestedOrientation = orientation;\n> + combinedTransform_ = data_->sensor_->computeTransform(&orientation);\n> + if (orientation != requestedOrientation)\n> + status = Adjusted;\n> + } else {\n> + combinedTransform_ = Transform::Rot0;\n> + }\n> +\n> /* Only 2 streams available. */\n> if (config_.size() > kMaxStreams) {\n> config_.resize(kMaxStreams);\n> @@ -521,7 +608,10 @@ public:\n> int queueRequestDevice(Camera *camera, Request *request) override;\n> \n> void bufferReady(FrameBuffer *buffer);\n> + void paramsBufferReady(FrameBuffer *buffer);\n> void statsBufferReady(FrameBuffer *buffer);\n> + void paramsComputed(unsigned int requestId);\n> + void statsProcessed(unsigned int requestId, const ControlList &metadata);\n> \n> bool match(DeviceEnumerator *enumerator) override;\n> \n> @@ -565,6 +655,10 @@ private:\n> pipe.stream = nullptr;\n> }\n> \n> + MaliC55FrameInfo *findFrameInfo(FrameBuffer *buffer);\n> + MaliC55FrameInfo *findFrameInfo(Request *request);\n> + void tryComplete(MaliC55FrameInfo *info);\n> +\n> int configureRawStream(MaliC55CameraData *data,\n> const StreamConfiguration &config,\n> V4L2SubdeviceFormat &subdevFormat);\n> @@ -574,7 +668,7 @@ private:\n> \n> void applyScalerCrop(Camera *camera, const ControlList &controls);\n> \n> - void registerMaliCamera(std::unique_ptr data,\n> + bool registerMaliCamera(std::unique_ptr data,\n> const std::string &name);\n> bool registerTPGCamera(MediaLink *link);\n> bool registerSensorCamera(MediaLink *link);\n> @@ -590,6 +684,8 @@ private:\n> std::vector> paramsBuffers_;\n> std::queue availableParamsBuffers_;\n> \n> + std::map frameInfoMap_;\n> +\n> std::array pipes_;\n> \n> bool dsFitted_;\n> @@ -838,6 +934,13 @@ int PipelineHandlerMaliC55::configure(Camera *camera,\n> if (ret)\n> return ret;\n> \n> + if (data->sensor_) {\n> + ret = data->sensor_->setFormat(&subdevFormat,\n> + maliConfig->combinedTransform());\n> + if (ret)\n> + return ret;\n> + }\n> +\n> if (data->csi_) {\n> ret = data->csi_->setFormat(0, &subdevFormat);\n> if (ret)\n> @@ -940,7 +1043,34 @@ int PipelineHandlerMaliC55::configure(Camera *camera,\n> return ret;\n> }\n> \n> - data->updateControls();\n> + if (!data->ipa_)\n> + return 0;\n> +\n> + /* We need to inform the IPA of the sensor configuration */\n> + ipa::mali_c55::IPAConfigInfo ipaConfig{};\n> +\n> + ret = data->sensor_->sensorInfo(&ipaConfig.sensorInfo);\n> + if (ret)\n> + return ret;\n> +\n> + ipaConfig.sensorControls = data->sensor_->controls();\n> +\n> + /*\n> + * And we also need to tell the IPA the bayerOrder of the data (as\n> + * affected by any flips that we've configured)\n> + */\n> + const Transform &combinedTransform = maliConfig->combinedTransform();\n> + BayerFormat::Order bayerOrder = data->sensor_->bayerOrder(combinedTransform);\n> +\n> + ControlInfoMap ipaControls;\n> + ret = data->ipa_->configure(ipaConfig, utils::to_underlying(bayerOrder),\n> + &ipaControls);\n> + if (ret) {\n> + LOG(MaliC55, Error) << \"Failed to configure IPA\";\n> + return ret;\n> + }\n> +\n> + data->updateControls(ipaControls);\n> \n> return 0;\n> }\n> @@ -954,8 +1084,10 @@ int PipelineHandlerMaliC55::exportFrameBuffers(Camera *camera, Stream *stream,\n> return pipe->cap->exportBuffers(count, buffers);\n> }\n> \n> -void PipelineHandlerMaliC55::freeBuffers([[maybe_unused]] Camera *camera)\n> +void PipelineHandlerMaliC55::freeBuffers(Camera *camera)\n> {\n> + MaliC55CameraData *data = cameraData(camera);\n> +\n> while (!availableStatsBuffers_.empty())\n> availableStatsBuffers_.pop();\n> while (!availableParamsBuffers_.empty())\n> @@ -964,11 +1096,18 @@ void PipelineHandlerMaliC55::freeBuffers([[maybe_unused]] Camera *camera)\n> statsBuffers_.clear();\n> paramsBuffers_.clear();\n> \n> + if (data->ipa_) {\n> + data->ipa_->unmapBuffers(data->ipaStatBuffers_);\n> + data->ipa_->unmapBuffers(data->ipaParamBuffers_);\n> + }\n> + data->ipaStatBuffers_.clear();\n> + data->ipaParamBuffers_.clear();\n> +\n> if (stats_->releaseBuffers())\n> LOG(MaliC55, Error) << \"Failed to release stats buffers\";\n> \n> if (params_->releaseBuffers())\n> - LOG(MaliC55, Error) << \"Failed to release stats buffers\";\n> + LOG(MaliC55, Error) << \"Failed to release params buffers\";\n\nMinor fix could be mentioned in the commit message, no specific need to\npull to it's own patch, but this is independent from the main $COMMIT.\n\n> \n> return;\n> }\n> @@ -976,6 +1115,7 @@ void PipelineHandlerMaliC55::freeBuffers([[maybe_unused]] Camera *camera)\n> int PipelineHandlerMaliC55::allocateBuffers(Camera *camera)\n> {\n> MaliC55CameraData *data = cameraData(camera);\n> + unsigned int ipaBufferId = 1;\n> unsigned int bufferCount;\n> int ret;\n> \n> @@ -988,27 +1128,51 @@ int PipelineHandlerMaliC55::allocateBuffers(Camera *camera)\n> if (ret < 0)\n> return ret;\n> \n> - for (std::unique_ptr &buffer : statsBuffers_)\n> + for (std::unique_ptr &buffer : statsBuffers_) {\n> + buffer->setCookie(ipaBufferId++);\n> + data->ipaStatBuffers_.emplace_back(buffer->cookie(),\n> + buffer->planes());\n> availableStatsBuffers_.push(buffer.get());\n> + }\n> \n> ret = params_->allocateBuffers(bufferCount, ¶msBuffers_);\n> if (ret < 0)\n> return ret;\n> \n> - for (std::unique_ptr &buffer : paramsBuffers_)\n> + for (std::unique_ptr &buffer : paramsBuffers_) {\n> + buffer->setCookie(ipaBufferId++);\n> + data->ipaParamBuffers_.emplace_back(buffer->cookie(),\n> + buffer->planes());\n> availableParamsBuffers_.push(buffer.get());\n> + }\n> +\n> + if (data->ipa_) {\n> + data->ipa_->mapBuffers(data->ipaStatBuffers_, true);\n> + data->ipa_->mapBuffers(data->ipaParamBuffers_, false);\n> + }\n> \n> return 0;\n> }\n> \n> int PipelineHandlerMaliC55::start(Camera *camera, [[maybe_unused]] const ControlList *controls)\n> {\n> + MaliC55CameraData *data = cameraData(camera);\n> int ret;\n> \n> ret = allocateBuffers(camera);\n> if (ret)\n> return ret;\n> \n> + if (data->ipa_) {\n> + ret = data->ipa_->start();\n> + if (ret) {\n> + LOG(MaliC55, Error)\n> + << \"Failed to start IPA\" << camera->id();\n> + freeBuffers(camera);\n> + return ret;\n> + }\n> + }\n> +\n> for (MaliC55Pipe &pipe : pipes_) {\n> if (!pipe.stream)\n> continue;\n> @@ -1018,6 +1182,8 @@ int PipelineHandlerMaliC55::start(Camera *camera, [[maybe_unused]] const Control\n> ret = pipe.cap->importBuffers(stream->configuration().bufferCount);\n> if (ret) {\n> LOG(MaliC55, Error) << \"Failed to import buffers\";\n> + if (data->ipa_)\n> + data->ipa_->stop();\n> freeBuffers(camera);\n> return ret;\n> }\n> @@ -1025,6 +1191,8 @@ int PipelineHandlerMaliC55::start(Camera *camera, [[maybe_unused]] const Control\n> ret = pipe.cap->streamOn();\n> if (ret) {\n> LOG(MaliC55, Error) << \"Failed to start stream\";\n> + if (data->ipa_)\n> + data->ipa_->stop();\n> freeBuffers(camera);\n> return ret;\n> }\n> @@ -1034,6 +1202,9 @@ int PipelineHandlerMaliC55::start(Camera *camera, [[maybe_unused]] const Control\n> if (ret) {\n> LOG(MaliC55, Error) << \"Failed to start stats stream\";\n> \n> + if (data->ipa_)\n> + data->ipa_->stop();\n> +\n> for (MaliC55Pipe &pipe : pipes_) {\n> if (pipe.stream)\n> pipe.cap->streamOff();\n> @@ -1048,6 +1219,8 @@ int PipelineHandlerMaliC55::start(Camera *camera, [[maybe_unused]] const Control\n> LOG(MaliC55, Error) << \"Failed to start params stream\";\n> \n> stats_->streamOff();\n> + if (data->ipa_)\n> + data->ipa_->stop();\n> \n\n\nI think I've said this on later patches, because I'm going backwards in\ntime ... but the cleanup helpers might help a lot here. But maybe that's\nsomething to do 'on top' anyway.\n\n\n> for (MaliC55Pipe &pipe : pipes_) {\n> if (pipe.stream)\n> @@ -1058,11 +1231,19 @@ int PipelineHandlerMaliC55::start(Camera *camera, [[maybe_unused]] const Control\n> return ret;\n> }\n> \n> + ret = isp_->setFrameStartEnabled(true);\n> + if (ret)\n> + LOG(MaliC55, Error) << \"Failed to enable frame start events\";\n> +\n> return 0;\n> }\n> \n> -void PipelineHandlerMaliC55::stopDevice([[maybe_unused]] Camera *camera)\n> +void PipelineHandlerMaliC55::stopDevice(Camera *camera)\n> {\n> + MaliC55CameraData *data = cameraData(camera);\n> +\n> + isp_->setFrameStartEnabled(false);\n> +\n> for (MaliC55Pipe &pipe : pipes_) {\n> if (!pipe.stream)\n> continue;\n> @@ -1073,6 +1254,8 @@ void PipelineHandlerMaliC55::stopDevice([[maybe_unused]] Camera *camera)\n> \n> stats_->streamOff();\n> params_->streamOff();\n> + if (data->ipa_)\n> + data->ipa_->stop();\n> freeBuffers(camera);\n> }\n> \n> @@ -1174,64 +1357,179 @@ void PipelineHandlerMaliC55::applyScalerCrop(Camera *camera,\n> \n> int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request)\n> {\n> - FrameBuffer *statsBuffer;\n> - int ret;\n> + MaliC55CameraData *data = cameraData(camera);\n> +\n> + /* Do not run the IPA if the TPG is in use. */\n> + if (!data->ipa_) {\n> + MaliC55FrameInfo frameInfo;\n> + frameInfo.request = request;\n> + frameInfo.statBuffer = nullptr;\n> + frameInfo.paramBuffer = nullptr;\n> + frameInfo.paramsDone = true;\n> + frameInfo.statsDone = true;\n> +\n> + frameInfoMap_[request->sequence()] = frameInfo;\n> +\n> + for (auto &[stream, buffer] : request->buffers()) {\n> + MaliC55Pipe *pipe = pipeFromStream(data, stream);\n> +\n> + pipe->cap->queueBuffer(buffer);\n> + }\n> +\n> + return 0;\n> + }\n\nAll of the 'if (!data->ipa_)' conditions make me wonder if we shouldn't\nsomehow abstract out and make a distinct 'no op' IPA for TPG which does\nthings for TPG only.\n\nBut the amount of work I could envisage in doing so, or trying to make a\ngeneric no-op IPA with the same interface ... in a way that actually\nsaves code would probably end up adding more complexity than \"if !ipa\"\nhandling...\n\n\n> \n> if (availableStatsBuffers_.empty()) {\n> LOG(MaliC55, Error) << \"Stats buffer underrun\";\n> return -ENOENT;\n> }\n> \n> - statsBuffer = availableStatsBuffers_.front();\n> + if (availableParamsBuffers_.empty()) {\n> + LOG(MaliC55, Error) << \"Params buffer underrun\";\n> + return -ENOENT;\n> + }\n> +\n> + MaliC55FrameInfo frameInfo;\n> + frameInfo.request = request;\n> +\n> + frameInfo.statBuffer = availableStatsBuffers_.front();\n> availableStatsBuffers_.pop();\n> + frameInfo.paramBuffer = availableParamsBuffers_.front();\n> + availableParamsBuffers_.pop();\n> \n> - /*\n> - * We need to associate the Request to this buffer even though it's a\n> - * purely internal one because we will need to use request->sequence()\n> - * later.\n> - */\n> - statsBuffer->_d()->setRequest(request);\n> + frameInfo.paramsDone = false;\n> + frameInfo.statsDone = false;\n> \n> - for (auto &[stream, buffer] : request->buffers()) {\n> - MaliC55Pipe *pipe = pipeFromStream(cameraData(camera), stream);\n> + frameInfoMap_[request->sequence()] = frameInfo;\n> \n> - ret = pipe->cap->queueBuffer(buffer);\n> - if (ret)\n> - return ret;\n> + data->ipa_->queueRequest(request->sequence(), request->controls());\n> + data->ipa_->fillParams(request->sequence(),\n> + frameInfo.paramBuffer->cookie());\n> +\n> + return 0;\n> +}\n> +\n> +MaliC55FrameInfo *PipelineHandlerMaliC55::findFrameInfo(Request *request)\n> +{\n> + for (auto &[sequence, info] : frameInfoMap_) {\n> + if (info.request == request)\n> + return &info;\n> }\n> \n> - /*\n> - * Some controls need to be applied immediately, as in example,\n> - * the ScalerCrop one.\n> - *\n> - * \\todo Move it buffer queue time (likely after the IPA has filled in\n> - * the parameters buffer) once we have plumbed the IPA loop in.\n> - */\n> - applyScalerCrop(camera, request->controls());\n> + return nullptr;\n> +}\n> \n> - ret = stats_->queueBuffer(statsBuffer);\n> - if (ret)\n> - return ret;\n> +MaliC55FrameInfo *PipelineHandlerMaliC55::findFrameInfo(FrameBuffer *buffer)\n> +{\n> + for (auto &[sequence, info] : frameInfoMap_) {\n> + if (info.paramBuffer == buffer ||\n> + info.statBuffer == buffer)\n> + return &info;\n> + }\n> \n> - return 0;\n> + return nullptr;\n> +}\n> +\n> +void PipelineHandlerMaliC55::tryComplete(MaliC55FrameInfo *info)\n> +{\n> + if (!info->paramsDone)\n> + return;\n> + if (!info->statsDone)\n> + return;\n> +\n> + Request *request = info->request;\n> + if (request->hasPendingBuffers())\n> + return;\n> +\n> + if (info->statBuffer)\n> + availableStatsBuffers_.push(info->statBuffer);\n> + if (info->paramBuffer)\n> + availableParamsBuffers_.push(info->paramBuffer);\n> +\n> + frameInfoMap_.erase(request->sequence());\n> +\n> + completeRequest(request);\n> }\n> \n> void PipelineHandlerMaliC55::bufferReady(FrameBuffer *buffer)\n> {\n> Request *request = buffer->request();\n> + MaliC55FrameInfo *info = findFrameInfo(request);\n> + ASSERT(info);\n> \n> if (completeBuffer(request, buffer))\n> - completeRequest(request);\n> + tryComplete(info);\n> +}\n> +\n> +void PipelineHandlerMaliC55::paramsBufferReady(FrameBuffer *buffer)\n> +{\n> + MaliC55FrameInfo *info = findFrameInfo(buffer);\n> + ASSERT(info);\n> +\n> + info->paramsDone = true;\n> +\n> + tryComplete(info);\n> }\n> \n> void PipelineHandlerMaliC55::statsBufferReady(FrameBuffer *buffer)\n> {\n> - availableStatsBuffers_.push(buffer);\n> + MaliC55FrameInfo *info = findFrameInfo(buffer);\n> + ASSERT(info);\n> +\n> + Request *request = info->request;\n> + MaliC55CameraData *data = cameraData(request->_d()->camera());\n> +\n> + ControlList sensorControls = data->delayedCtrls_->get(buffer->metadata().sequence);\n> +\n> + data->ipa_->processStats(request->sequence(), buffer->cookie(),\n> + sensorControls);\n> }\n> \n> -void PipelineHandlerMaliC55::registerMaliCamera(std::unique_ptr data,\n> +void PipelineHandlerMaliC55::paramsComputed(unsigned int requestId)\n> +{\n> + MaliC55FrameInfo &frameInfo = frameInfoMap_[requestId];\n> + Request *request = frameInfo.request;\n> + MaliC55CameraData *data = cameraData(request->_d()->camera());\n> +\n> + /*\n> + * Queue buffers for stats and params, then queue buffers to the capture\n> + * video devices.\n> + */\n> +\n> + frameInfo.paramBuffer->_d()->metadata().planes()[0].bytesused =\n> + sizeof(struct mali_c55_params_buffer);\n> + params_->queueBuffer(frameInfo.paramBuffer);\n> + stats_->queueBuffer(frameInfo.statBuffer);\n> +\n> + for (auto &[stream, buffer] : request->buffers()) {\n> + MaliC55Pipe *pipe = pipeFromStream(data, stream);\n> +\n> + pipe->cap->queueBuffer(buffer);\n> + }\n> +}\n> +\n> +void PipelineHandlerMaliC55::statsProcessed(unsigned int requestId,\n> + const ControlList &metadata)\n> +{\n> + MaliC55FrameInfo &frameInfo = frameInfoMap_[requestId];\n> +\n> + frameInfo.statsDone = true;\n> + frameInfo.request->metadata().merge(metadata);\n> +\n> + tryComplete(&frameInfo);\n> +}\n> +\n> +bool PipelineHandlerMaliC55::registerMaliCamera(std::unique_ptr data,\n> const std::string &name)\n> {\n> + if (data->loadIPA())\n> + return false;\n> +\n> + if (data->ipa_) {\n> + data->ipa_->statsProcessed.connect(this, &PipelineHandlerMaliC55::statsProcessed);\n> + data->ipa_->paramsComputed.connect(this, &PipelineHandlerMaliC55::paramsComputed);\n> + }\n> +\n> std::set streams{ &data->frStream_ };\n> if (dsFitted_)\n> streams.insert(&data->dsStream_);\n> @@ -1239,6 +1537,8 @@ void PipelineHandlerMaliC55::registerMaliCamera(std::unique_ptr std::shared_ptr camera = Camera::create(std::move(data),\n> name, streams);\n> registerCamera(std::move(camera));\n> +\n> + return true;\n> }\n> \n> /*\n> @@ -1264,9 +1564,7 @@ bool PipelineHandlerMaliC55::registerTPGCamera(MediaLink *link)\n> if (data->init())\n> return false;\n> \n> - registerMaliCamera(std::move(data), name);\n> -\n> - return true;\n> + return registerMaliCamera(std::move(data), name);\n> }\n> \n> /*\n> @@ -1293,9 +1591,27 @@ bool PipelineHandlerMaliC55::registerSensorCamera(MediaLink *ispLink)\n> return false;\n> \n> data->properties_ = data->sensor_->properties();\n> - data->updateControls();\n> \n> - registerMaliCamera(std::move(data), sensor->name());\n> + /*\n> + * \\todo Read delay values from the sensor itself or from a\n> + * a sensor database. For now use generic values taken from\n> + * the Raspberry Pi and listed as 'generic values'.\n> + */\n> + std::unordered_map params = {\n> + { V4L2_CID_ANALOGUE_GAIN, { 1, false } },\n> + { V4L2_CID_EXPOSURE, { 2, false } },\n> + };\n\nWe *REALLY* need to fix this for all the libipa pipeline handlers.\n\nSo much so - that I would probably be tempted to say \"Hey - lets not\nmerge more pipeline handlers that hardcode the wrong delays until we fix\nthe CameraSensorHelper to also report the delays per sensor.\n\nIt's \"just\" a case of adding a delay map in the CameraSensorHelper\ninstances right?\n\nIt shouldn't be hard... but no one is doing it.\n\n\nIf that were done then I'd give this patch my RB tag ;-)\n\n\n\n> +\n> + data->delayedCtrls_ =\n> + std::make_unique(data->sensor_->device(),\n> + params);\n> + isp_->frameStart.connect(data->delayedCtrls_.get(),\n> + &DelayedControls::applyControls);\n> +\n> + /* \\todo: Init properties. */\n> +\n> + if (!registerMaliCamera(std::move(data), sensor->name()))\n> + return false;\n> }\n> \n> return true;\n> @@ -1362,6 +1678,7 @@ bool PipelineHandlerMaliC55::match(DeviceEnumerator *enumerator)\n> }\n> \n> stats_->bufferReady.connect(this, &PipelineHandlerMaliC55::statsBufferReady);\n> + params_->bufferReady.connect(this, &PipelineHandlerMaliC55::paramsBufferReady);\n> \n> ispSink = isp_->entity()->getPadByIndex(0);\n> if (!ispSink || ispSink->links().empty()) {\n> -- \n> 2.34.1\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 4A3EDC32DE\n\tfor ;\n\tWed, 9 Oct 2024 21:25:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CE71863538;\n\tWed, 9 Oct 2024 23:25:26 +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 9378A618C5\n\tfor ;\n\tWed, 9 Oct 2024 23:25:25 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BC390594;\n\tWed, 9 Oct 2024 23:23:47 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"EE/76wzz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728509027;\n\tbh=JbCnRFZZEFWQmcH/CdSjawjw8So8Nsv/v+DlTfWp1xU=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=EE/76wzzVYu8R2/Yxcz4L9+wQB1j0s/dwRn/WhM25wdLDIPhIRhGaNkaP6wXdcEDH\n\tJqdR5GvXUvUXYaLnyxdguN2XVtTdJS83qml+v9ZdvXCBXRwbRnqcJVYBUOx+uky2an\n\tnZpfmNhut6kAzdg0vD0GQIIQXd49MSrX/3vzzXbE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240709144950.3277837-6-dan.scally@ideasonboard.com>","References":"<20240709144950.3277837-1-dan.scally@ideasonboard.com>\n\t<20240709144950.3277837-6-dan.scally@ideasonboard.com>","Subject":"Re: [PATCH v2 05/10] mali-c55: Plumb the IPA module in","From":"Kieran Bingham ","Cc":"Jacopo Mondi ,\n\tNayden Kanchev ","To":"Daniel Scally ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 09 Oct 2024 22:25:22 +0100","Message-ID":"<172850912219.532453.3576191739872514782@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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\" "}}]