[{"id":13925,"web_url":"https://patchwork.libcamera.org/comment/13925/","msgid":"<20201126154123.GP3905@pendragon.ideasonboard.com>","date":"2020-11-26T15:41:23","subject":"Re: [libcamera-devel] [PATCH v4 26/37] libcamera: pipeline,\n\tipa: raspberrypi: Use new data definition","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Fri, Nov 06, 2020 at 07:36:56PM +0900, Paul Elder wrote:\n> Now that we can generate custom functions and data structures with mojo,\n> switch the raspberrypi pipeline handler and IPA to use the custom data\n> structures as defined in the mojom data definition file.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> Changes in v4:\n> - rename Controls to controls\n> - use the renamed header (raspberrypi_ipa_interface.h)\n> \n> Changes in v3:\n> - use ipa::rpi namespace\n> - rebase on the RPi namespace patch series newly merged into master\n> \n> Changes in v2:\n> - rebased on \"libcamera: pipeline: ipa: raspberrypi: Rework drop frame\n> signalling\"\n> - use newly customized RPi IPA interface\n> ---\n> include/libcamera/ipa/raspberrypi.h | 40 ++--\n> src/ipa/raspberrypi/raspberrypi.cpp | 160 ++++++---------\n> .../pipeline/raspberrypi/raspberrypi.cpp | 193 +++++++++---------\n> 3 files changed, 183 insertions(+), 210 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index df30b4a2..259f943e 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -28,24 +28,28 @@ enum BufferMask {\n> static constexpr unsigned int MaxLsGridSize = 32 << 10;\n> \n> /* List of controls handled by the Raspberry Pi IPA */\n> -static const ControlInfoMap Controls = {\n> -\t{ &controls::AeEnable, ControlInfo(false, true) },\n> -\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n> -\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> -\t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> -\t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> -\t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> -\t{ &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n> -\t{ &controls::AwbEnable, ControlInfo(false, true) },\n> -\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> -\t{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> -\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> -\t{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> -\t{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n> -\t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> -\t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> -\t{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> -};\n> +static ControlInfoMap controls;\n> +\n> +inline void initializeRPiControls()\n> +{\n> +\tcontrols = {\n> +\t\t{ &controls::AeEnable, ControlInfo(false, true) },\n> +\t\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n> +\t\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> +\t\t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> +\t\t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> +\t\t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> +\t\t{ &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n> +\t\t{ &controls::AwbEnable, ControlInfo(false, true) },\n> +\t\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> +\t\t{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> +\t\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> +\t\t{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> +\t\t{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n> +\t\t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> +\t\t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n\n-16,+15. Looks like a mishandled rebase perhaps ? Could you double-check\nthe values to make sure they're all fine ?\n\n> +\t};\n> +}\n\nGiven that the IPA doesn't actually need this, I'd like to move this map\nto the pipeline handler. However, the generate serdes code uses this.\nCan it be dropped ? Require all pipeline handlers to define a\nControlInfoMap in a shared header isn't the right way forward. Maybe\nwith your rework of the control serdes code, following our last\ndiscussion regarding the dependency between control lists and control\ninfo maps, this will not be needed anymore ?\n\n> } /* namespace RPi */\n> \n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index f338ff8b..316da7bd 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -19,6 +19,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -60,7 +61,7 @@ constexpr unsigned int DefaultExposureTime = 20000;\n> \n> LOG_DEFINE_CATEGORY(IPARPI)\n> \n> -class IPARPi : public IPAInterface\n> +class IPARPi : public ipa::rpi::IPARPiInterface\n\nI think I commented in a previous patch that ipa::rpi::Interface may be\ngood enough, without a need to repeat the prefix, but if I haven't, now\nI've expressed it :-)\n\n> {\n> public:\n> \tIPARPi()\n> @@ -68,6 +69,7 @@ public:\n> \t\t frameCount_(0), checkCount_(0), mistrustCount_(0),\n> \t\t lsTable_(nullptr)\n> \t{\n> +\t\tRPi::initializeRPiControls();\n\nThe IPA actually doesn't need this.\n\n> \t}\n> \n> \t~IPARPi()\n> @@ -82,12 +84,14 @@ public:\n> \n> \tvoid configure(const CameraSensorInfo &sensorInfo,\n> \t\t const std::map &streamConfig,\n> -\t\t const std::map &entityControls,\n> -\t\t const IPAOperationData &data,\n> -\t\t IPAOperationData *response) override;\n> +\t\t const std::map &entityControls,\n\nOuch, that hurts, having to make a copy of the ControlInfoMap in the\ncaller just for the purpose of adding it to this map. Is this something\nthat can be fixed ?\n\n> +\t\t const ipa::rpi::ConfigInput &data,\n\nIt feels a bit weird passing some data through dedicated parameters, and\nsome data in a miscellaneous config structure. I'd ask if we shouldn't\nmove everything to ipa::rpi::ConfigInput, but I fear we'll then have\nmore of the above copy issue ?\n\nThis seems to be an annoying limitation of the IPA IPC framework. If\nthis happens to be simple to fix (which I doubt) it would be nice to\naddress the issue, but I assume we should do so on top. Still, if you\nalready have ideas on how this could be handled, we can discuss it.\n\n> +\t\t ipa::rpi::ConfigOutput *response) override;\n> \tvoid mapBuffers(const std::vector &buffers) override;\n> \tvoid unmapBuffers(const std::vector &ids) override;\n> -\tvoid processEvent(const IPAOperationData &event) override;\n> +\tvoid signalStatReady(const uint32_t bufferId) override;\n> +\tvoid signalQueueRequest(const ControlList &controls) override;\n> +\tvoid signalIspPrepare(const ipa::rpi::IspPreparePayload &data) override;\n\nI like this :-)\n\n> \n> private:\n> \tvoid setMode(const CameraSensorInfo &sensorInfo);\n> @@ -144,6 +148,11 @@ private:\n> \n> \t/* LS table allocation passed in from the pipeline handler. */\n> \tFileDescriptor lsTableHandle_;\n> +\t/*\n> +\t * LS table allocation passed in from the pipeline handler,\n> +\t * in the context of the pipeline handler.\n> +\t */\n> +\tint32_t lsTableHandlePH_;\n> \tvoid *lsTable_;\n> };\n> \n> @@ -193,15 +202,13 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)\n> \n> void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t\t [[maybe_unused]] const std::map &streamConfig,\n> -\t\t const std::map &entityControls,\n> -\t\t const IPAOperationData &ipaConfig,\n> -\t\t IPAOperationData *result)\n> +\t\t const std::map &entityControls,\n> +\t\t const ipa::rpi::ConfigInput &ipaConfig,\n> +\t\t ipa::rpi::ConfigOutput *result)\n> {\n> \tif (entityControls.empty())\n> \t\treturn;\n> \n> -\tresult->operation = 0;\n> -\n> \tunicamCtrls_ = entityControls.at(0);\n> \tispCtrls_ = entityControls.at(1);\n> \n> @@ -225,32 +232,28 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t\thelper_->GetDelays(exposureDelay, gainDelay);\n> \t\tsensorMetadata = helper_->SensorEmbeddedDataPresent();\n> \n> -\t\tresult->data.push_back(gainDelay);\n> -\t\tresult->data.push_back(exposureDelay);\n> -\t\tresult->data.push_back(sensorMetadata);\n> -\n> -\t\tresult->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;\n> +\t\tresult->op_ |= ipa::rpi::RPI_IPA_CONFIG_STAGGERED_WRITE;\n> +\t\tresult->staggeredWriteResult_.gainDelay_ = gainDelay;\n> +\t\tresult->staggeredWriteResult_.exposureDelay_ = exposureDelay;\n> +\t\tresult->staggeredWriteResult_.sensorMetadata_ = sensorMetadata;\n> \t}\n> \n> \t/* Re-assemble camera mode using the sensor info. */\n> \tsetMode(sensorInfo);\n> \n> -\t/*\n> -\t * The ipaConfig.data always gives us the user transform first. Note that\n> -\t * this will always make the LS table pointer (if present) element 1.\n> -\t */\n> -\tmode_.transform = static_cast(ipaConfig.data[0]);\n> +\tmode_.transform = static_cast(ipaConfig.transform_);\n\nSo much nicer :-)\n\n> \n> \t/* Store the lens shading table pointer and handle if available. */\n> -\tif (ipaConfig.operation & RPi::IPA_CONFIG_LS_TABLE) {\n> +\tif (ipaConfig.op_ & ipa::rpi::RPI_IPA_CONFIG_LS_TABLE) {\n> \t\t/* Remove any previous table, if there was one. */\n> \t\tif (lsTable_) {\n> \t\t\tmunmap(lsTable_, RPi::MaxLsGridSize);\n> \t\t\tlsTable_ = nullptr;\n> \t\t}\n> \n> -\t\t/* Map the LS table buffer into user space (now element 1). */\n> -\t\tlsTableHandle_ = FileDescriptor(ipaConfig.data[1]);\n> +\t\t/* Map the LS table buffer into user space. */\n> +\t\tlsTableHandle_ = FileDescriptor(ipaConfig.lsTableHandle_);\n> +\t\tlsTableHandlePH_ = ipaConfig.lsTableHandleStatic_;\n> \t\tif (lsTableHandle_.isValid()) {\n> \t\t\tlsTable_ = mmap(nullptr, RPi::MaxLsGridSize, PROT_READ | PROT_WRITE,\n> \t\t\t\t\tMAP_SHARED, lsTableHandle_.fd(), 0);\n> @@ -272,18 +275,15 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t */\n> \tframeCount_ = 0;\n> \tcheckCount_ = 0;\n> -\tunsigned int dropFrame = 0;\n> +\tresult->op_ |= ipa::rpi::RPI_IPA_CONFIG_DROP_FRAMES;\n> \tif (controllerInit_) {\n> -\t\tdropFrame = helper_->HideFramesModeSwitch();\n> +\t\tresult->dropFrameCount_ = helper_->HideFramesModeSwitch();\n> \t\tmistrustCount_ = helper_->MistrustFramesModeSwitch();\n> \t} else {\n> -\t\tdropFrame = helper_->HideFramesStartup();\n> +\t\tresult->dropFrameCount_ = helper_->HideFramesStartup();\n> \t\tmistrustCount_ = helper_->MistrustFramesStartup();\n> \t}\n> \n> -\tresult->data.push_back(dropFrame);\n> -\tresult->operation |= RPi::IPA_CONFIG_DROP_FRAMES;\n> -\n> \t/* These zero values mean not program anything (unless overwritten). */\n> \tstruct AgcStatus agcStatus;\n> \tagcStatus.shutter_time = 0.0;\n> @@ -308,9 +308,9 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \tif (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {\n> \t\tControlList ctrls(unicamCtrls_);\n> \t\tapplyAGC(&agcStatus, ctrls);\n> -\t\tresult->controls.push_back(ctrls);\n> \n> -\t\tresult->operation |= RPi::IPA_CONFIG_SENSOR;\n> +\t\tresult->op_ |= ipa::rpi::RPI_IPA_CONFIG_SENSOR;\n> +\t\tresult->controls_ = ctrls;\n\nstd::move() should be more efficient.\n\n> \t}\n> \n> \tlastMode_ = mode_;\n> @@ -348,56 +348,38 @@ void IPARPi::unmapBuffers(const std::vector &ids)\n> \t}\n> }\n> \n> -void IPARPi::processEvent(const IPAOperationData &event)\n> +void IPARPi::signalStatReady(const uint32_t bufferId)\n> {\n> -\tswitch (event.operation) {\n> -\tcase RPi::IPA_EVENT_SIGNAL_STAT_READY: {\n> -\t\tunsigned int bufferId = event.data[0];\n> -\n> -\t\tif (++checkCount_ != frameCount_) /* assert here? */\n> -\t\t\tLOG(IPARPI, Error) << \"WARNING: Prepare/Process mismatch!!!\";\n> -\t\tif (frameCount_ > mistrustCount_)\n> -\t\t\tprocessStats(bufferId);\n> -\n> -\t\treportMetadata();\n> -\n> -\t\tIPAOperationData op;\n> -\t\top.operation = RPi::IPA_ACTION_STATS_METADATA_COMPLETE;\n> -\t\top.data = { bufferId & RPi::BufferMask::ID };\n> -\t\top.controls = { libcameraMetadata_ };\n> -\t\tqueueFrameAction.emit(0, op);\n> -\t\tbreak;\n> -\t}\n> +\tif (++checkCount_ != frameCount_) /* assert here? */\n> +\t\tLOG(IPARPI, Error) << \"WARNING: Prepare/Process mismatch!!!\";\n> +\tif (frameCount_ > mistrustCount_)\n> +\t\tprocessStats(bufferId);\n> \n> -\tcase RPi::IPA_EVENT_SIGNAL_ISP_PREPARE: {\n> -\t\tunsigned int embeddedbufferId = event.data[0];\n> -\t\tunsigned int bayerbufferId = event.data[1];\n> +\treportMetadata();\n> \n> -\t\t/*\n> -\t\t * At start-up, or after a mode-switch, we may want to\n> -\t\t * avoid running the control algos for a few frames in case\n> -\t\t * they are \"unreliable\".\n> -\t\t */\n> -\t\tprepareISP(embeddedbufferId);\n> -\t\tframeCount_++;\n> -\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 & RPi::BufferMask::ID };\n> -\t\tqueueFrameAction.emit(0, op);\n> -\t\tbreak;\n> -\t}\n> +\tstatsMetadataComplete.emit(bufferId & RPi::BufferMask::ID, libcameraMetadata_);\n> +}\n> \n> -\tcase RPi::IPA_EVENT_QUEUE_REQUEST: {\n> -\t\tqueueRequest(event.controls[0]);\n> -\t\tbreak;\n> -\t}\n> +void IPARPi::signalQueueRequest(const ControlList &controls)\n> +{\n> +\tqueueRequest(controls);\n> +}\n> \n> -\tdefault:\n> -\t\tLOG(IPARPI, Error) << \"Unknown event \" << event.operation;\n> -\t\tbreak;\n> -\t}\n> +void IPARPi::signalIspPrepare(const ipa::rpi::IspPreparePayload &data)\n> +{\n> +\tunsigned int embeddedbufferId = data.embeddedbufferId_;\n> +\tunsigned int bayerbufferId = data.bayerbufferId_;\n> +\n> +\t/*\n> +\t * At start-up, or after a mode-switch, we may want to\n> +\t * avoid running the control algos for a few frames in case\n> +\t * they are \"unreliable\".\n> +\t */\n> +\tprepareISP(embeddedbufferId);\n> +\tframeCount_++;\n> +\n> +\t/* Ready to push the input buffer into the ISP. */\n> +\trunIsp.emit(bayerbufferId & RPi::BufferMask::ID);\n> }\n> \n> void IPARPi::reportMetadata()\n> @@ -498,6 +480,8 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t/* Clear the return metadata buffer. */\n> \tlibcameraMetadata_.clear();\n> \n> +\tLOG(IPARPI, Info) << \"Request ctrl length: \" << controls.size();\n> +\n\nDebugging leftover ?\n\n> \tfor (auto const &ctrl : controls) {\n> \t\tLOG(IPARPI, Info) << \"Request ctrl: \"\n> \t\t\t\t << controls::controls.at(ctrl.first)->name()\n> @@ -715,10 +699,7 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \n> void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> {\n> -\tIPAOperationData op;\n> -\top.operation = RPi::IPA_ACTION_EMBEDDED_COMPLETE;\n> -\top.data = { bufferId & RPi::BufferMask::ID };\n> -\tqueueFrameAction.emit(0, op);\n> +\tembeddedComplete.emit(bufferId & RPi::BufferMask::ID);\n> }\n> \n> void IPARPi::prepareISP(unsigned int bufferId)\n> @@ -779,12 +760,8 @@ void IPARPi::prepareISP(unsigned int bufferId)\n> \t\tif (dpcStatus)\n> \t\t\tapplyDPC(dpcStatus, ctrls);\n> \n> -\t\tif (!ctrls.empty()) {\n> -\t\t\tIPAOperationData op;\n> -\t\t\top.operation = RPi::IPA_ACTION_V4L2_SET_ISP;\n> -\t\t\top.controls.push_back(ctrls);\n> -\t\t\tqueueFrameAction.emit(0, op);\n> -\t\t}\n> +\t\tif (!ctrls.empty())\n> +\t\t\tsetIsp.emit(ctrls);\n> \t}\n> }\n> \n> @@ -840,10 +817,7 @@ void IPARPi::processStats(unsigned int bufferId)\n> \t\tControlList ctrls(unicamCtrls_);\n> \t\tapplyAGC(&agcStatus, ctrls);\n> \n> -\t\tIPAOperationData op;\n> -\t\top.operation = RPi::IPA_ACTION_V4L2_SET_STAGGERED;\n> -\t\top.controls.push_back(ctrls);\n> -\t\tqueueFrameAction.emit(0, op);\n> +\t\tsetStaggered.emit(ctrls);\n> \t}\n> }\n> \n> @@ -1073,7 +1047,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> \t\t.grid_width = w,\n> \t\t.grid_stride = w,\n> \t\t.grid_height = h,\n> -\t\t.dmabuf = lsTableHandle_.fd(),\n> +\t\t.dmabuf = lsTableHandlePH_,\n\nThis feels like a bit of a hack. How about simplifying it, by dropping\nlsTableHandlePH_ here and filling the field in the pipeline handler\ninstead ?\n\n> \t\t.ref_transform = 0,\n> \t\t.corner_sampled = 1,\n> \t\t.gain_format = GAIN_FORMAT_U4P10\n> @@ -1150,9 +1124,9 @@ const struct IPAModuleInfo ipaModuleInfo = {\n> \t\"raspberrypi\",\n> };\n> \n> -struct ipa_context *ipaCreate()\n> +IPAInterface *ipaCreate()\n> {\n> -\treturn new IPAInterfaceWrapper(std::make_unique());\n> +\treturn new IPARPi();\n> }\n> \n> } /* extern \"C\" */\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 0a60442c..0e9ff16d 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -16,7 +16,9 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -144,7 +146,11 @@ public:\n> \tint loadIPA();\n> \tint configureIPA(const CameraConfiguration *config);\n> \n> -\tvoid queueFrameAction(unsigned int frame, const IPAOperationData &action);\n> +\tvoid statsMetadataComplete(uint32_t bufferId, const ControlList &controls);\n> +\tvoid runIsp(uint32_t bufferId);\n> +\tvoid embeddedComplete(uint32_t bufferId);\n> +\tvoid setIsp(const ControlList &controls);\n> +\tvoid setStaggered(const ControlList &controls);\n> \n> \t/* bufferComplete signal handlers. */\n> \tvoid unicamBufferDequeue(FrameBuffer *buffer);\n> @@ -156,6 +162,8 @@ public:\n> \tvoid handleExternalBuffer(FrameBuffer *buffer, RPi::Stream *stream);\n> \tvoid handleState();\n> \n> +\tstd::unique_ptr ipa_;\n> +\n> \tCameraSensor *sensor_;\n> \t/* Array of Unicam and ISP device streams and associated buffers/streams. */\n> \tRPi::Device unicam_;\n> @@ -448,6 +456,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)\n> \t: PipelineHandler(manager), unicam_(nullptr), isp_(nullptr)\n> {\n> +\tRPi::initializeRPiControls();\n> }\n> \n> CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> @@ -936,7 +945,7 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> \t}\n> \n> \t/* Register the controls that the Raspberry Pi IPA can handle. */\n> -\tdata->controlInfo_ = RPi::Controls;\n> +\tdata->controlInfo_ = RPi::controls;\n> \t/* Initialize the camera properties. */\n> \tdata->properties_ = data->sensor_->properties();\n> \n> @@ -1114,11 +1123,16 @@ void RPiCameraData::frameStarted(uint32_t sequence)\n> \n> int RPiCameraData::loadIPA()\n> {\n> -\tipa_ = IPAManager::createIPA(pipe_, 1, 1);\n> +\tipa_ = IPAManager::createIPA(pipe_, 1, 1);\n> +\n> \tif (!ipa_)\n> \t\treturn -ENOENT;\n> \n> -\tipa_->queueFrameAction.connect(this, &RPiCameraData::queueFrameAction);\n> +\tipa_->statsMetadataComplete.connect(this, &RPiCameraData::statsMetadataComplete);\n> +\tipa_->runIsp.connect(this, &RPiCameraData::runIsp);\n> +\tipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete);\n> +\tipa_->setIsp.connect(this, &RPiCameraData::setIsp);\n> +\tipa_->setStaggered.connect(this, &RPiCameraData::setStaggered);\n> \n> \tIPASettings settings{\n> \t\t.configurationFile = ipa_->configurationFile(sensor_->model() + \".json\")\n> @@ -1134,8 +1148,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> \t\tstatic_cast(config);\n> \n> \tstd::map streamConfig;\n> -\tstd::map entityControls;\n> -\tIPAOperationData ipaConfig = {};\n> +\tstd::map entityControls;\n> +\tipa::rpi::ConfigInput ipaConfig;\n> \n> \t/* Get the device format to pass to the IPA. */\n> \tV4L2DeviceFormat sensorFormat;\n> @@ -1155,7 +1169,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> \tentityControls.emplace(1, isp_[Isp::Input].dev()->controls());\n> \n> \t/* Always send the user transform to the IPA. */\n> -\tipaConfig.data = { static_cast(config->transform) };\n> +\tipaConfig.transform_ = static_cast(config->transform);\n> \n> \t/* Allocate the lens shading table via dmaHeap and pass to the IPA. */\n> \tif (!lsTable_.isValid()) {\n> @@ -1164,8 +1178,9 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> \t\t\treturn -ENOMEM;\n> \n> \t\t/* Allow the IPA to mmap the LS table via the file descriptor. */\n> -\t\tipaConfig.operation = RPi::IPA_CONFIG_LS_TABLE;\n> -\t\tipaConfig.data.push_back(static_cast(lsTable_.fd()));\n> +\t\tipaConfig.op_ |= ipa::rpi::RPI_IPA_CONFIG_LS_TABLE;\n> +\t\tipaConfig.lsTableHandle_ = lsTable_;\n> +\t\tipaConfig.lsTableHandleStatic_ = lsTable_.fd();\n> \t}\n> \n> \t/* We store the CameraSensorInfo for digital zoom calculations. */\n> @@ -1176,34 +1191,35 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> \t}\n> \n> \t/* Ready the IPA - it must know about the sensor resolution. */\n> -\tIPAOperationData result;\n> +\tipa::rpi::ConfigOutput result;\n> \n> \tipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,\n> \t\t\t&result);\n> \n> -\tunsigned int resultIdx = 0;\n> -\tif (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {\n> +\tif (result.op_ & ipa::rpi::RPI_IPA_CONFIG_STAGGERED_WRITE) {\n> \t\t/*\n> \t\t * Setup our staggered control writer with the sensor default\n> \t\t * gain and exposure delays.\n> \t\t */\n> \t\tif (!staggeredCtrl_) {\n> \t\t\tstaggeredCtrl_.init(unicam_[Unicam::Image].dev(),\n> -\t\t\t\t\t { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },\n> -\t\t\t\t\t { V4L2_CID_EXPOSURE, result.data[resultIdx++] } });\n> -\t\t\tsensorMetadata_ = result.data[resultIdx++];\n> +\t\t\t\t\t{ { V4L2_CID_ANALOGUE_GAIN,\n> +\t\t\t\t\tresult.staggeredWriteResult_.gainDelay_ },\n> +\t\t\t\t\t{ V4L2_CID_EXPOSURE,\n> +\t\t\t\t\tresult.staggeredWriteResult_.exposureDelay_ } });\n> +\t\t\tsensorMetadata_ = result.staggeredWriteResult_.sensorMetadata_;\n> \t\t}\n> \t}\n> \n> -\tif (result.operation & RPi::IPA_CONFIG_SENSOR) {\n> -\t\tconst ControlList &ctrls = result.controls[0];\n> +\tif (result.op_ & ipa::rpi::RPI_IPA_CONFIG_SENSOR) {\n> +\t\tconst ControlList &ctrls = result.controls_;\n> \t\tif (!staggeredCtrl_.set(ctrls))\n> \t\t\tLOG(RPI, Error) << \"V4L2 staggered set failed\";\n> \t}\n> \n> -\tif (result.operation & RPi::IPA_CONFIG_DROP_FRAMES) {\n> +\tif (result.op_ & ipa::rpi::RPI_IPA_CONFIG_DROP_FRAMES) {\n> \t\t/* Configure the number of dropped frames required on startup. */\n> -\t\tdropFrameCount_ = result.data[resultIdx++];\n> +\t\tdropFrameCount_ = result.dropFrameCount_;\n> \t}\n> \n> \t/*\n> @@ -1222,90 +1238,75 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> \treturn 0;\n> }\n> \n> -void RPiCameraData::queueFrameAction([[maybe_unused]] unsigned int frame,\n> -\t\t\t\t const IPAOperationData &action)\n> +void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)\n> {\n> -\t/*\n> -\t * The following actions can be handled when the pipeline handler is in\n> -\t * a stopped state.\n> -\t */\n> -\tswitch (action.operation) {\n> -\tcase RPi::IPA_ACTION_V4L2_SET_STAGGERED: {\n> -\t\tconst ControlList &controls = action.controls[0];\n> -\t\tif (!staggeredCtrl_.set(controls))\n> -\t\t\tLOG(RPI, Error) << \"V4L2 staggered set failed\";\n> -\t\tgoto done;\n> -\t}\n> +\tif (state_ == State::Stopped)\n> +\t\thandleState();\n> \n> -\tcase RPi::IPA_ACTION_V4L2_SET_ISP: {\n> -\t\tControlList controls = action.controls[0];\n> -\t\tisp_[Isp::Input].dev()->setControls(&controls);\n> -\t\tgoto done;\n> -\t}\n> -\t}\n> +\tFrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> \n> -\tif (state_ == State::Stopped)\n> -\t\tgoto done;\n> +\thandleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> +\n> +\t/* Fill the Request metadata buffer with what the IPA has provided */\n> +\tRequest *request = requestQueue_.front();\n> +\trequest->metadata() = std::move(controls);\n> \n> \t/*\n> -\t * The following actions must not be handled when the pipeline handler\n> -\t * is in a stopped state.\n> +\t * Also update the ScalerCrop in the metadata with what we actually\n> +\t * used. But we must first rescale that from ISP (camera mode) pixels\n> +\t * back into sensor native pixels.\n> +\t *\n> +\t * Sending this information on every frame may be helpful.\n> \t */\n> -\tswitch (action.operation) {\n> -\tcase RPi::IPA_ACTION_STATS_METADATA_COMPLETE: {\n> -\t\tunsigned int bufferId = action.data[0];\n> -\t\tFrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> +\tif (updateScalerCrop_) {\n> +\t\tupdateScalerCrop_ = false;\n> +\t\tscalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> +\t\t\t\tsensorInfo_.outputSize);\n> +\t\tscalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n> +\t}\n> +\trequest->metadata().set(controls::ScalerCrop, scalerCrop_);\n> \n> -\t\thandleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> +\tstate_ = State::IpaComplete;\n> \n> -\t\t/* Fill the Request metadata buffer with what the IPA has provided */\n> -\t\tRequest *request = requestQueue_.front();\n> -\t\trequest->metadata() = std::move(action.controls[0]);\n> +\thandleState();\n> +}\n> \n> -\t\t/*\n> -\t\t * Also update the ScalerCrop in the metadata with what we actually\n> -\t\t * used. But we must first rescale that from ISP (camera mode) pixels\n> -\t\t * back into sensor native pixels.\n> -\t\t *\n> -\t\t * Sending this information on every frame may be helpful.\n> -\t\t */\n> -\t\tif (updateScalerCrop_) {\n> -\t\t\tupdateScalerCrop_ = false;\n> -\t\t\tscalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> -\t\t\t\t\t\t\tsensorInfo_.outputSize);\n> -\t\t\tscalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n> -\t\t}\n> -\t\trequest->metadata().set(controls::ScalerCrop, scalerCrop_);\n> +void RPiCameraData::runIsp(uint32_t bufferId)\n> +{\n> +\tif (state_ == State::Stopped)\n> +\t\thandleState();\n> \n> -\t\tstate_ = State::IpaComplete;\n> -\t\tbreak;\n> -\t}\n> +\tFrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);\n> \n> -\tcase RPi::IPA_ACTION_EMBEDDED_COMPLETE: {\n> -\t\tunsigned int bufferId = action.data[0];\n> -\t\tFrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);\n> -\t\thandleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> -\t\tbreak;\n> -\t}\n> +\tLOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << bufferId\n> +\t\t\t<< \", timestamp: \" << buffer->metadata().timestamp;\n> \n> -\tcase RPi::IPA_ACTION_RUN_ISP: {\n> -\t\tunsigned int bufferId = action.data[0];\n> -\t\tFrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);\n> +\tisp_[Isp::Input].queueBuffer(buffer);\n> +\tispOutputCount_ = 0;\n> +\thandleState();\n> +}\n> \n> -\t\tLOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << bufferId\n> -\t\t\t\t<< \", timestamp: \" << buffer->metadata().timestamp;\n> +void RPiCameraData::embeddedComplete(uint32_t bufferId)\n> +{\n> +\tif (state_ == State::Stopped)\n> +\t\thandleState();\n> \n> -\t\tisp_[Isp::Input].queueBuffer(buffer);\n> -\t\tispOutputCount_ = 0;\n> -\t\tbreak;\n> -\t}\n> +\tFrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);\n> +\thandleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> +\thandleState();\n> +}\n> \n> -\tdefault:\n> -\t\tLOG(RPI, Error) << \"Unknown action \" << action.operation;\n> -\t\tbreak;\n> -\t}\n> +void RPiCameraData::setIsp(const ControlList &controls)\n> +{\n> +\tControlList ctrls = controls;\n> +\tisp_[Isp::Input].dev()->setControls(&ctrls);\n> +\thandleState();\n> +}\n> \n> -done:\n> +void RPiCameraData::setStaggered(const ControlList &controls)\n> +{\n> +\tif (!staggeredCtrl_.set(controls))\n> +\t\tLOG(RPI, Error) << \"V4L2 staggered set failed\";\n> \thandleState();\n> }\n> \n> @@ -1405,10 +1406,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n> \t * application until after the IPA signals so.\n> \t */\n> \tif (stream == &isp_[Isp::Stats]) {\n> -\t\tIPAOperationData op;\n> -\t\top.operation = RPi::IPA_EVENT_SIGNAL_STAT_READY;\n> -\t\top.data = { RPi::BufferMask::STATS | static_cast(index) };\n> -\t\tipa_->processEvent(op);\n> +\t\tipa_->signalStatReady(RPi::BufferMask::STATS | static_cast(index));\n> \t} else {\n> \t\t/* Any other ISP output can be handed back to the application now. */\n> \t\thandleStreamBuffer(buffer, stream);\n> @@ -1581,7 +1579,6 @@ void RPiCameraData::checkRequestCompleted()\n> void RPiCameraData::tryRunPipeline()\n> {\n> \tFrameBuffer *bayerBuffer, *embeddedBuffer;\n> -\tIPAOperationData op;\n> \n> \t/* If any of our request or buffer queues are empty, we cannot proceed. */\n> \tif (state_ != State::Idle || requestQueue_.empty() ||\n> @@ -1661,9 +1658,7 @@ void RPiCameraData::tryRunPipeline()\n> \t * queue the ISP output buffer listed in the request to start the HW\n> \t * pipeline.\n> \t */\n> -\top.operation = RPi::IPA_EVENT_QUEUE_REQUEST;\n> -\top.controls = { request->controls() };\n> -\tipa_->processEvent(op);\n> +\tipa_->signalQueueRequest(request->controls());\n> \n> \t/* Ready to use the buffers, pop them off the queue. */\n> \tbayerQueue_.pop();\n> @@ -1679,10 +1674,10 @@ void RPiCameraData::tryRunPipeline()\n> \t\t\t<< \" Bayer buffer id: \" << bayerId\n> \t\t\t<< \" Embedded buffer id: \" << embeddedId;\n> \n> -\top.operation = RPi::IPA_EVENT_SIGNAL_ISP_PREPARE;\n> -\top.data = { RPi::BufferMask::EMBEDDED_DATA | embeddedId,\n> -\t\t RPi::BufferMask::BAYER_DATA | bayerId };\n> -\tipa_->processEvent(op);\n> +\tipa::rpi::IspPreparePayload ispPrepare;\n> +\tispPrepare.embeddedbufferId_ = RPi::BufferMask::EMBEDDED_DATA | embeddedId;\n> +\tispPrepare.bayerbufferId_ = RPi::BufferMask::BAYER_DATA | bayerId;\n> +\tipa_->signalIspPrepare(ispPrepare);\n> }\n> \n> void RPiCameraData::tryFlushQueues()","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 BB956BE08A\n\tfor ;\n\tThu, 26 Nov 2020 15:41:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 32F8863474;\n\tThu, 26 Nov 2020 16:41:34 +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 7BCA86344B\n\tfor ;\n\tThu, 26 Nov 2020 16:41:32 +0100 (CET)","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 DBDE8A1B;\n\tThu, 26 Nov 2020 16:41:31 +0100 (CET)"],"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=\"DJYor6Md\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1606405292;\n\tbh=BiZTW5QKtL3KXbXpEwxZhICNoQSALrXMzlWTYMbh9V4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DJYor6Md8IcIVSmVFWPIEvZGFNtsu1iRmXg66Pgm0wgDUFnoTp/70OFBXU0cX0LrK\n\tAn2iZRIlgxHuCvzBdCRe/y9XgTlVINrZbWgH9OqYvOucn7Q4b8PHWv6T/swjd0CMt0\n\tIoPjCTdTSjazjlhkI1I6aeGQ8Fix6gayP7o8cOSc=","Date":"Thu, 26 Nov 2020 17:41:23 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"<20201126154123.GP3905@pendragon.ideasonboard.com>","References":"<20201106103707.49660-1-paul.elder@ideasonboard.com>\n\t<20201106103707.49660-27-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201106103707.49660-27-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 26/37] libcamera: pipeline,\n\tipa: raspberrypi: Use new data definition","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14073,"web_url":"https://patchwork.libcamera.org/comment/14073/","msgid":"<20201205034912.GE2050@pyrite.rasen.tech>","date":"2020-12-05T03:49:12","subject":"Re: [libcamera-devel] [PATCH v4 26/37] libcamera: pipeline,\n\tipa: raspberrypi: Use new data definition","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the review.\n\nOn Thu, Nov 26, 2020 at 05:41:23PM +0200, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> On Fri, Nov 06, 2020 at 07:36:56PM +0900, Paul Elder wrote:\n> > Now that we can generate custom functions and data structures with mojo,\n> > switch the raspberrypi pipeline handler and IPA to use the custom data\n> > structures as defined in the mojom data definition file.\n> > \n> > Signed-off-by: Paul Elder \n> > \n> > ---\n> > Changes in v4:\n> > - rename Controls to controls\n> > - use the renamed header (raspberrypi_ipa_interface.h)\n> > \n> > Changes in v3:\n> > - use ipa::rpi namespace\n> > - rebase on the RPi namespace patch series newly merged into master\n> > \n> > Changes in v2:\n> > - rebased on \"libcamera: pipeline: ipa: raspberrypi: Rework drop frame\n> > signalling\"\n> > - use newly customized RPi IPA interface\n> > ---\n> > include/libcamera/ipa/raspberrypi.h | 40 ++--\n> > src/ipa/raspberrypi/raspberrypi.cpp | 160 ++++++---------\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 193 +++++++++---------\n> > 3 files changed, 183 insertions(+), 210 deletions(-)\n> > \n> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > index df30b4a2..259f943e 100644\n> > --- a/include/libcamera/ipa/raspberrypi.h\n> > +++ b/include/libcamera/ipa/raspberrypi.h\n> > @@ -28,24 +28,28 @@ enum BufferMask {\n> > static constexpr unsigned int MaxLsGridSize = 32 << 10;\n> > \n> > /* List of controls handled by the Raspberry Pi IPA */\n> > -static const ControlInfoMap Controls = {\n> > -\t{ &controls::AeEnable, ControlInfo(false, true) },\n> > -\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n> > -\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> > -\t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> > -\t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> > -\t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> > -\t{ &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n> > -\t{ &controls::AwbEnable, ControlInfo(false, true) },\n> > -\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> > -\t{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> > -\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> > -\t{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> > -\t{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n> > -\t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > -\t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> > -\t{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> > -};\n> > +static ControlInfoMap controls;\n> > +\n> > +inline void initializeRPiControls()\n> > +{\n> > +\tcontrols = {\n> > +\t\t{ &controls::AeEnable, ControlInfo(false, true) },\n> > +\t\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n> > +\t\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> > +\t\t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> > +\t\t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> > +\t\t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> > +\t\t{ &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n> > +\t\t{ &controls::AwbEnable, ControlInfo(false, true) },\n> > +\t\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> > +\t\t{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> > +\t\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> > +\t\t{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> > +\t\t{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n> > +\t\t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > +\t\t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> \n> -16,+15. Looks like a mishandled rebase perhaps ? Could you double-check\n> the values to make sure they're all fine ?\n> \n> > +\t};\n> > +}\n> \n> Given that the IPA doesn't actually need this, I'd like to move this map\n> to the pipeline handler. However, the generate serdes code uses this.\n\nThe generated serdes code uses it in the event that ControlList doesn't\ncontain the ControlInfoMap. If ControlList will always have a\nControlInfoMap, or we can skip serdes if it doesn't, then yes, we can\nremove the map from here.\n\n> Can it be dropped ? Require all pipeline handlers to define a\n> ControlInfoMap in a shared header isn't the right way forward. Maybe\n> with your rework of the control serdes code, following our last\n> discussion regarding the dependency between control lists and control\n> info maps, this will not be needed anymore ?\n\nYeah. I'll look into to between v5 and v6.\n\n> > } /* namespace RPi */\n> > \n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index f338ff8b..316da7bd 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -19,6 +19,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > \n> > @@ -60,7 +61,7 @@ constexpr unsigned int DefaultExposureTime = 20000;\n> > \n> > LOG_DEFINE_CATEGORY(IPARPI)\n> > \n> > -class IPARPi : public IPAInterface\n> > +class IPARPi : public ipa::rpi::IPARPiInterface\n> \n> I think I commented in a previous patch that ipa::rpi::Interface may be\n> good enough, without a need to repeat the prefix, but if I haven't, now\n> I've expressed it :-)\n\nWell atm we don't require the interface definitions to declare a\nnamespace. If we do though, then we can drop the prefix, which would\nalso simplify parsing code :)\n\nShould we require it then?\n\n> > {\n> > public:\n> > \tIPARPi()\n> > @@ -68,6 +69,7 @@ public:\n> > \t\t frameCount_(0), checkCount_(0), mistrustCount_(0),\n> > \t\t lsTable_(nullptr)\n> > \t{\n> > +\t\tRPi::initializeRPiControls();\n> \n> The IPA actually doesn't need this.\n\natm this is necessary to appease the serdes, unless ControlLists are\nguaranteed to have a ControlInfoMap.\n\n> > \t}\n> > \n> > \t~IPARPi()\n> > @@ -82,12 +84,14 @@ public:\n> > \n> > \tvoid configure(const CameraSensorInfo &sensorInfo,\n> > \t\t const std::map &streamConfig,\n> > -\t\t const std::map &entityControls,\n> > -\t\t const IPAOperationData &data,\n> > -\t\t IPAOperationData *response) override;\n> > +\t\t const std::map &entityControls,\n> \n> Ouch, that hurts, having to make a copy of the ControlInfoMap in the\n> caller just for the purpose of adding it to this map. Is this something\n> that can be fixed ?\n\nPerhaps. We could make it so that all non-arithmetic members of\nmap/vector in function parameters are references. That should work fine,\nright..?\n\nI'll give it a shot between v5 and v6.\n\n> > +\t\t const ipa::rpi::ConfigInput &data,\n> \n> It feels a bit weird passing some data through dedicated parameters, and\n> some data in a miscellaneous config structure. I'd ask if we shouldn't\n> move everything to ipa::rpi::ConfigInput, but I fear we'll then have\n> more of the above copy issue ?\n\nWell, that was just because for now I'm copying the old raspberrypi IPA\ninterface as-is.\n\n> This seems to be an annoying limitation of the IPA IPC framework. If\n> this happens to be simple to fix (which I doubt) it would be nice to\n> address the issue, but I assume we should do so on top. Still, if you\n> already have ideas on how this could be handled, we can discuss it.\n> \n> > +\t\t ipa::rpi::ConfigOutput *response) override;\n> > \tvoid mapBuffers(const std::vector &buffers) override;\n> > \tvoid unmapBuffers(const std::vector &ids) override;\n> > -\tvoid processEvent(const IPAOperationData &event) override;\n> > +\tvoid signalStatReady(const uint32_t bufferId) override;\n> > +\tvoid signalQueueRequest(const ControlList &controls) override;\n> > +\tvoid signalIspPrepare(const ipa::rpi::IspPreparePayload &data) override;\n> \n> I like this :-)\n> \n> > \n> > private:\n> > \tvoid setMode(const CameraSensorInfo &sensorInfo);\n> > @@ -144,6 +148,11 @@ private:\n> > \n> > \t/* LS table allocation passed in from the pipeline handler. */\n> > \tFileDescriptor lsTableHandle_;\n> > +\t/*\n> > +\t * LS table allocation passed in from the pipeline handler,\n> > +\t * in the context of the pipeline handler.\n> > +\t */\n> > +\tint32_t lsTableHandlePH_;\n> > \tvoid *lsTable_;\n> > };\n> > \n> > @@ -193,15 +202,13 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)\n> > \n> > void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > \t\t [[maybe_unused]] const std::map &streamConfig,\n> > -\t\t const std::map &entityControls,\n> > -\t\t const IPAOperationData &ipaConfig,\n> > -\t\t IPAOperationData *result)\n> > +\t\t const std::map &entityControls,\n> > +\t\t const ipa::rpi::ConfigInput &ipaConfig,\n> > +\t\t ipa::rpi::ConfigOutput *result)\n> > {\n> > \tif (entityControls.empty())\n> > \t\treturn;\n> > \n> > -\tresult->operation = 0;\n> > -\n> > \tunicamCtrls_ = entityControls.at(0);\n> > \tispCtrls_ = entityControls.at(1);\n> > \n> > @@ -225,32 +232,28 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > \t\thelper_->GetDelays(exposureDelay, gainDelay);\n> > \t\tsensorMetadata = helper_->SensorEmbeddedDataPresent();\n> > \n> > -\t\tresult->data.push_back(gainDelay);\n> > -\t\tresult->data.push_back(exposureDelay);\n> > -\t\tresult->data.push_back(sensorMetadata);\n> > -\n> > -\t\tresult->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;\n> > +\t\tresult->op_ |= ipa::rpi::RPI_IPA_CONFIG_STAGGERED_WRITE;\n> > +\t\tresult->staggeredWriteResult_.gainDelay_ = gainDelay;\n> > +\t\tresult->staggeredWriteResult_.exposureDelay_ = exposureDelay;\n> > +\t\tresult->staggeredWriteResult_.sensorMetadata_ = sensorMetadata;\n> > \t}\n> > \n> > \t/* Re-assemble camera mode using the sensor info. */\n> > \tsetMode(sensorInfo);\n> > \n> > -\t/*\n> > -\t * The ipaConfig.data always gives us the user transform first. Note that\n> > -\t * this will always make the LS table pointer (if present) element 1.\n> > -\t */\n> > -\tmode_.transform = static_cast(ipaConfig.data[0]);\n> > +\tmode_.transform = static_cast(ipaConfig.transform_);\n> \n> So much nicer :-)\n> \n> > \n> > \t/* Store the lens shading table pointer and handle if available. */\n> > -\tif (ipaConfig.operation & RPi::IPA_CONFIG_LS_TABLE) {\n> > +\tif (ipaConfig.op_ & ipa::rpi::RPI_IPA_CONFIG_LS_TABLE) {\n> > \t\t/* Remove any previous table, if there was one. */\n> > \t\tif (lsTable_) {\n> > \t\t\tmunmap(lsTable_, RPi::MaxLsGridSize);\n> > \t\t\tlsTable_ = nullptr;\n> > \t\t}\n> > \n> > -\t\t/* Map the LS table buffer into user space (now element 1). */\n> > -\t\tlsTableHandle_ = FileDescriptor(ipaConfig.data[1]);\n> > +\t\t/* Map the LS table buffer into user space. */\n> > +\t\tlsTableHandle_ = FileDescriptor(ipaConfig.lsTableHandle_);\n> > +\t\tlsTableHandlePH_ = ipaConfig.lsTableHandleStatic_;\n> > \t\tif (lsTableHandle_.isValid()) {\n> > \t\t\tlsTable_ = mmap(nullptr, RPi::MaxLsGridSize, PROT_READ | PROT_WRITE,\n> > \t\t\t\t\tMAP_SHARED, lsTableHandle_.fd(), 0);\n> > @@ -272,18 +275,15 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > \t */\n> > \tframeCount_ = 0;\n> > \tcheckCount_ = 0;\n> > -\tunsigned int dropFrame = 0;\n> > +\tresult->op_ |= ipa::rpi::RPI_IPA_CONFIG_DROP_FRAMES;\n> > \tif (controllerInit_) {\n> > -\t\tdropFrame = helper_->HideFramesModeSwitch();\n> > +\t\tresult->dropFrameCount_ = helper_->HideFramesModeSwitch();\n> > \t\tmistrustCount_ = helper_->MistrustFramesModeSwitch();\n> > \t} else {\n> > -\t\tdropFrame = helper_->HideFramesStartup();\n> > +\t\tresult->dropFrameCount_ = helper_->HideFramesStartup();\n> > \t\tmistrustCount_ = helper_->MistrustFramesStartup();\n> > \t}\n> > \n> > -\tresult->data.push_back(dropFrame);\n> > -\tresult->operation |= RPi::IPA_CONFIG_DROP_FRAMES;\n> > -\n> > \t/* These zero values mean not program anything (unless overwritten). */\n> > \tstruct AgcStatus agcStatus;\n> > \tagcStatus.shutter_time = 0.0;\n> > @@ -308,9 +308,9 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > \tif (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {\n> > \t\tControlList ctrls(unicamCtrls_);\n> > \t\tapplyAGC(&agcStatus, ctrls);\n> > -\t\tresult->controls.push_back(ctrls);\n> > \n> > -\t\tresult->operation |= RPi::IPA_CONFIG_SENSOR;\n> > +\t\tresult->op_ |= ipa::rpi::RPI_IPA_CONFIG_SENSOR;\n> > +\t\tresult->controls_ = ctrls;\n> \n> std::move() should be more efficient.\n> \n> > \t}\n> > \n> > \tlastMode_ = mode_;\n> > @@ -348,56 +348,38 @@ void IPARPi::unmapBuffers(const std::vector &ids)\n> > \t}\n> > }\n> > \n> > -void IPARPi::processEvent(const IPAOperationData &event)\n> > +void IPARPi::signalStatReady(const uint32_t bufferId)\n> > {\n> > -\tswitch (event.operation) {\n> > -\tcase RPi::IPA_EVENT_SIGNAL_STAT_READY: {\n> > -\t\tunsigned int bufferId = event.data[0];\n> > -\n> > -\t\tif (++checkCount_ != frameCount_) /* assert here? */\n> > -\t\t\tLOG(IPARPI, Error) << \"WARNING: Prepare/Process mismatch!!!\";\n> > -\t\tif (frameCount_ > mistrustCount_)\n> > -\t\t\tprocessStats(bufferId);\n> > -\n> > -\t\treportMetadata();\n> > -\n> > -\t\tIPAOperationData op;\n> > -\t\top.operation = RPi::IPA_ACTION_STATS_METADATA_COMPLETE;\n> > -\t\top.data = { bufferId & RPi::BufferMask::ID };\n> > -\t\top.controls = { libcameraMetadata_ };\n> > -\t\tqueueFrameAction.emit(0, op);\n> > -\t\tbreak;\n> > -\t}\n> > +\tif (++checkCount_ != frameCount_) /* assert here? */\n> > +\t\tLOG(IPARPI, Error) << \"WARNING: Prepare/Process mismatch!!!\";\n> > +\tif (frameCount_ > mistrustCount_)\n> > +\t\tprocessStats(bufferId);\n> > \n> > -\tcase RPi::IPA_EVENT_SIGNAL_ISP_PREPARE: {\n> > -\t\tunsigned int embeddedbufferId = event.data[0];\n> > -\t\tunsigned int bayerbufferId = event.data[1];\n> > +\treportMetadata();\n> > \n> > -\t\t/*\n> > -\t\t * At start-up, or after a mode-switch, we may want to\n> > -\t\t * avoid running the control algos for a few frames in case\n> > -\t\t * they are \"unreliable\".\n> > -\t\t */\n> > -\t\tprepareISP(embeddedbufferId);\n> > -\t\tframeCount_++;\n> > -\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 & RPi::BufferMask::ID };\n> > -\t\tqueueFrameAction.emit(0, op);\n> > -\t\tbreak;\n> > -\t}\n> > +\tstatsMetadataComplete.emit(bufferId & RPi::BufferMask::ID, libcameraMetadata_);\n> > +}\n> > \n> > -\tcase RPi::IPA_EVENT_QUEUE_REQUEST: {\n> > -\t\tqueueRequest(event.controls[0]);\n> > -\t\tbreak;\n> > -\t}\n> > +void IPARPi::signalQueueRequest(const ControlList &controls)\n> > +{\n> > +\tqueueRequest(controls);\n> > +}\n> > \n> > -\tdefault:\n> > -\t\tLOG(IPARPI, Error) << \"Unknown event \" << event.operation;\n> > -\t\tbreak;\n> > -\t}\n> > +void IPARPi::signalIspPrepare(const ipa::rpi::IspPreparePayload &data)\n> > +{\n> > +\tunsigned int embeddedbufferId = data.embeddedbufferId_;\n> > +\tunsigned int bayerbufferId = data.bayerbufferId_;\n> > +\n> > +\t/*\n> > +\t * At start-up, or after a mode-switch, we may want to\n> > +\t * avoid running the control algos for a few frames in case\n> > +\t * they are \"unreliable\".\n> > +\t */\n> > +\tprepareISP(embeddedbufferId);\n> > +\tframeCount_++;\n> > +\n> > +\t/* Ready to push the input buffer into the ISP. */\n> > +\trunIsp.emit(bayerbufferId & RPi::BufferMask::ID);\n> > }\n> > \n> > void IPARPi::reportMetadata()\n> > @@ -498,6 +480,8 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > \t/* Clear the return metadata buffer. */\n> > \tlibcameraMetadata_.clear();\n> > \n> > +\tLOG(IPARPI, Info) << \"Request ctrl length: \" << controls.size();\n> > +\n> \n> Debugging leftover ?\n> \n> > \tfor (auto const &ctrl : controls) {\n> > \t\tLOG(IPARPI, Info) << \"Request ctrl: \"\n> > \t\t\t\t << controls::controls.at(ctrl.first)->name()\n> > @@ -715,10 +699,7 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > \n> > void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> > {\n> > -\tIPAOperationData op;\n> > -\top.operation = RPi::IPA_ACTION_EMBEDDED_COMPLETE;\n> > -\top.data = { bufferId & RPi::BufferMask::ID };\n> > -\tqueueFrameAction.emit(0, op);\n> > +\tembeddedComplete.emit(bufferId & RPi::BufferMask::ID);\n> > }\n> > \n> > void IPARPi::prepareISP(unsigned int bufferId)\n> > @@ -779,12 +760,8 @@ void IPARPi::prepareISP(unsigned int bufferId)\n> > \t\tif (dpcStatus)\n> > \t\t\tapplyDPC(dpcStatus, ctrls);\n> > \n> > -\t\tif (!ctrls.empty()) {\n> > -\t\t\tIPAOperationData op;\n> > -\t\t\top.operation = RPi::IPA_ACTION_V4L2_SET_ISP;\n> > -\t\t\top.controls.push_back(ctrls);\n> > -\t\t\tqueueFrameAction.emit(0, op);\n> > -\t\t}\n> > +\t\tif (!ctrls.empty())\n> > +\t\t\tsetIsp.emit(ctrls);\n> > \t}\n> > }\n> > \n> > @@ -840,10 +817,7 @@ void IPARPi::processStats(unsigned int bufferId)\n> > \t\tControlList ctrls(unicamCtrls_);\n> > \t\tapplyAGC(&agcStatus, ctrls);\n> > \n> > -\t\tIPAOperationData op;\n> > -\t\top.operation = RPi::IPA_ACTION_V4L2_SET_STAGGERED;\n> > -\t\top.controls.push_back(ctrls);\n> > -\t\tqueueFrameAction.emit(0, op);\n> > +\t\tsetStaggered.emit(ctrls);\n> > \t}\n> > }\n> > \n> > @@ -1073,7 +1047,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> > \t\t.grid_width = w,\n> > \t\t.grid_stride = w,\n> > \t\t.grid_height = h,\n> > -\t\t.dmabuf = lsTableHandle_.fd(),\n> > +\t\t.dmabuf = lsTableHandlePH_,\n> \n> This feels like a bit of a hack. How about simplifying it, by dropping\n> lsTableHandlePH_ here and filling the field in the pipeline handler\n> instead ?\n\nThis is also what the raspberrypi IPA was doing before.\n\n\nPaul\n> > \t\t.ref_transform = 0,\n> > \t\t.corner_sampled = 1,\n> > \t\t.gain_format = GAIN_FORMAT_U4P10\n> > @@ -1150,9 +1124,9 @@ const struct IPAModuleInfo ipaModuleInfo = {\n> > \t\"raspberrypi\",\n> > };\n> > \n> > -struct ipa_context *ipaCreate()\n> > +IPAInterface *ipaCreate()\n> > {\n> > -\treturn new IPAInterfaceWrapper(std::make_unique());\n> > +\treturn new IPARPi();\n> > }\n> > \n> > } /* extern \"C\" */\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 0a60442c..0e9ff16d 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -16,7 +16,9 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -144,7 +146,11 @@ public:\n> > \tint loadIPA();\n> > \tint configureIPA(const CameraConfiguration *config);\n> > \n> > -\tvoid queueFrameAction(unsigned int frame, const IPAOperationData &action);\n> > +\tvoid statsMetadataComplete(uint32_t bufferId, const ControlList &controls);\n> > +\tvoid runIsp(uint32_t bufferId);\n> > +\tvoid embeddedComplete(uint32_t bufferId);\n> > +\tvoid setIsp(const ControlList &controls);\n> > +\tvoid setStaggered(const ControlList &controls);\n> > \n> > \t/* bufferComplete signal handlers. */\n> > \tvoid unicamBufferDequeue(FrameBuffer *buffer);\n> > @@ -156,6 +162,8 @@ public:\n> > \tvoid handleExternalBuffer(FrameBuffer *buffer, RPi::Stream *stream);\n> > \tvoid handleState();\n> > \n> > +\tstd::unique_ptr ipa_;\n> > +\n> > \tCameraSensor *sensor_;\n> > \t/* Array of Unicam and ISP device streams and associated buffers/streams. */\n> > \tRPi::Device unicam_;\n> > @@ -448,6 +456,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)\n> > \t: PipelineHandler(manager), unicam_(nullptr), isp_(nullptr)\n> > {\n> > +\tRPi::initializeRPiControls();\n> > }\n> > \n> > CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > @@ -936,7 +945,7 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > \t}\n> > \n> > \t/* Register the controls that the Raspberry Pi IPA can handle. */\n> > -\tdata->controlInfo_ = RPi::Controls;\n> > +\tdata->controlInfo_ = RPi::controls;\n> > \t/* Initialize the camera properties. */\n> > \tdata->properties_ = data->sensor_->properties();\n> > \n> > @@ -1114,11 +1123,16 @@ void RPiCameraData::frameStarted(uint32_t sequence)\n> > \n> > int RPiCameraData::loadIPA()\n> > {\n> > -\tipa_ = IPAManager::createIPA(pipe_, 1, 1);\n> > +\tipa_ = IPAManager::createIPA(pipe_, 1, 1);\n> > +\n> > \tif (!ipa_)\n> > \t\treturn -ENOENT;\n> > \n> > -\tipa_->queueFrameAction.connect(this, &RPiCameraData::queueFrameAction);\n> > +\tipa_->statsMetadataComplete.connect(this, &RPiCameraData::statsMetadataComplete);\n> > +\tipa_->runIsp.connect(this, &RPiCameraData::runIsp);\n> > +\tipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete);\n> > +\tipa_->setIsp.connect(this, &RPiCameraData::setIsp);\n> > +\tipa_->setStaggered.connect(this, &RPiCameraData::setStaggered);\n> > \n> > \tIPASettings settings{\n> > \t\t.configurationFile = ipa_->configurationFile(sensor_->model() + \".json\")\n> > @@ -1134,8 +1148,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > \t\tstatic_cast(config);\n> > \n> > \tstd::map streamConfig;\n> > -\tstd::map entityControls;\n> > -\tIPAOperationData ipaConfig = {};\n> > +\tstd::map entityControls;\n> > +\tipa::rpi::ConfigInput ipaConfig;\n> > \n> > \t/* Get the device format to pass to the IPA. */\n> > \tV4L2DeviceFormat sensorFormat;\n> > @@ -1155,7 +1169,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > \tentityControls.emplace(1, isp_[Isp::Input].dev()->controls());\n> > \n> > \t/* Always send the user transform to the IPA. */\n> > -\tipaConfig.data = { static_cast(config->transform) };\n> > +\tipaConfig.transform_ = static_cast(config->transform);\n> > \n> > \t/* Allocate the lens shading table via dmaHeap and pass to the IPA. */\n> > \tif (!lsTable_.isValid()) {\n> > @@ -1164,8 +1178,9 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > \t\t\treturn -ENOMEM;\n> > \n> > \t\t/* Allow the IPA to mmap the LS table via the file descriptor. */\n> > -\t\tipaConfig.operation = RPi::IPA_CONFIG_LS_TABLE;\n> > -\t\tipaConfig.data.push_back(static_cast(lsTable_.fd()));\n> > +\t\tipaConfig.op_ |= ipa::rpi::RPI_IPA_CONFIG_LS_TABLE;\n> > +\t\tipaConfig.lsTableHandle_ = lsTable_;\n> > +\t\tipaConfig.lsTableHandleStatic_ = lsTable_.fd();\n> > \t}\n> > \n> > \t/* We store the CameraSensorInfo for digital zoom calculations. */\n> > @@ -1176,34 +1191,35 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > \t}\n> > \n> > \t/* Ready the IPA - it must know about the sensor resolution. */\n> > -\tIPAOperationData result;\n> > +\tipa::rpi::ConfigOutput result;\n> > \n> > \tipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,\n> > \t\t\t&result);\n> > \n> > -\tunsigned int resultIdx = 0;\n> > -\tif (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {\n> > +\tif (result.op_ & ipa::rpi::RPI_IPA_CONFIG_STAGGERED_WRITE) {\n> > \t\t/*\n> > \t\t * Setup our staggered control writer with the sensor default\n> > \t\t * gain and exposure delays.\n> > \t\t */\n> > \t\tif (!staggeredCtrl_) {\n> > \t\t\tstaggeredCtrl_.init(unicam_[Unicam::Image].dev(),\n> > -\t\t\t\t\t { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },\n> > -\t\t\t\t\t { V4L2_CID_EXPOSURE, result.data[resultIdx++] } });\n> > -\t\t\tsensorMetadata_ = result.data[resultIdx++];\n> > +\t\t\t\t\t{ { V4L2_CID_ANALOGUE_GAIN,\n> > +\t\t\t\t\tresult.staggeredWriteResult_.gainDelay_ },\n> > +\t\t\t\t\t{ V4L2_CID_EXPOSURE,\n> > +\t\t\t\t\tresult.staggeredWriteResult_.exposureDelay_ } });\n> > +\t\t\tsensorMetadata_ = result.staggeredWriteResult_.sensorMetadata_;\n> > \t\t}\n> > \t}\n> > \n> > -\tif (result.operation & RPi::IPA_CONFIG_SENSOR) {\n> > -\t\tconst ControlList &ctrls = result.controls[0];\n> > +\tif (result.op_ & ipa::rpi::RPI_IPA_CONFIG_SENSOR) {\n> > +\t\tconst ControlList &ctrls = result.controls_;\n> > \t\tif (!staggeredCtrl_.set(ctrls))\n> > \t\t\tLOG(RPI, Error) << \"V4L2 staggered set failed\";\n> > \t}\n> > \n> > -\tif (result.operation & RPi::IPA_CONFIG_DROP_FRAMES) {\n> > +\tif (result.op_ & ipa::rpi::RPI_IPA_CONFIG_DROP_FRAMES) {\n> > \t\t/* Configure the number of dropped frames required on startup. */\n> > -\t\tdropFrameCount_ = result.data[resultIdx++];\n> > +\t\tdropFrameCount_ = result.dropFrameCount_;\n> > \t}\n> > \n> > \t/*\n> > @@ -1222,90 +1238,75 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > \treturn 0;\n> > }\n> > \n> > -void RPiCameraData::queueFrameAction([[maybe_unused]] unsigned int frame,\n> > -\t\t\t\t const IPAOperationData &action)\n> > +void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)\n> > {\n> > -\t/*\n> > -\t * The following actions can be handled when the pipeline handler is in\n> > -\t * a stopped state.\n> > -\t */\n> > -\tswitch (action.operation) {\n> > -\tcase RPi::IPA_ACTION_V4L2_SET_STAGGERED: {\n> > -\t\tconst ControlList &controls = action.controls[0];\n> > -\t\tif (!staggeredCtrl_.set(controls))\n> > -\t\t\tLOG(RPI, Error) << \"V4L2 staggered set failed\";\n> > -\t\tgoto done;\n> > -\t}\n> > +\tif (state_ == State::Stopped)\n> > +\t\thandleState();\n> > \n> > -\tcase RPi::IPA_ACTION_V4L2_SET_ISP: {\n> > -\t\tControlList controls = action.controls[0];\n> > -\t\tisp_[Isp::Input].dev()->setControls(&controls);\n> > -\t\tgoto done;\n> > -\t}\n> > -\t}\n> > +\tFrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> > \n> > -\tif (state_ == State::Stopped)\n> > -\t\tgoto done;\n> > +\thandleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> > +\n> > +\t/* Fill the Request metadata buffer with what the IPA has provided */\n> > +\tRequest *request = requestQueue_.front();\n> > +\trequest->metadata() = std::move(controls);\n> > \n> > \t/*\n> > -\t * The following actions must not be handled when the pipeline handler\n> > -\t * is in a stopped state.\n> > +\t * Also update the ScalerCrop in the metadata with what we actually\n> > +\t * used. But we must first rescale that from ISP (camera mode) pixels\n> > +\t * back into sensor native pixels.\n> > +\t *\n> > +\t * Sending this information on every frame may be helpful.\n> > \t */\n> > -\tswitch (action.operation) {\n> > -\tcase RPi::IPA_ACTION_STATS_METADATA_COMPLETE: {\n> > -\t\tunsigned int bufferId = action.data[0];\n> > -\t\tFrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> > +\tif (updateScalerCrop_) {\n> > +\t\tupdateScalerCrop_ = false;\n> > +\t\tscalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> > +\t\t\t\tsensorInfo_.outputSize);\n> > +\t\tscalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n> > +\t}\n> > +\trequest->metadata().set(controls::ScalerCrop, scalerCrop_);\n> > \n> > -\t\thandleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> > +\tstate_ = State::IpaComplete;\n> > \n> > -\t\t/* Fill the Request metadata buffer with what the IPA has provided */\n> > -\t\tRequest *request = requestQueue_.front();\n> > -\t\trequest->metadata() = std::move(action.controls[0]);\n> > +\thandleState();\n> > +}\n> > \n> > -\t\t/*\n> > -\t\t * Also update the ScalerCrop in the metadata with what we actually\n> > -\t\t * used. But we must first rescale that from ISP (camera mode) pixels\n> > -\t\t * back into sensor native pixels.\n> > -\t\t *\n> > -\t\t * Sending this information on every frame may be helpful.\n> > -\t\t */\n> > -\t\tif (updateScalerCrop_) {\n> > -\t\t\tupdateScalerCrop_ = false;\n> > -\t\t\tscalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> > -\t\t\t\t\t\t\tsensorInfo_.outputSize);\n> > -\t\t\tscalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n> > -\t\t}\n> > -\t\trequest->metadata().set(controls::ScalerCrop, scalerCrop_);\n> > +void RPiCameraData::runIsp(uint32_t bufferId)\n> > +{\n> > +\tif (state_ == State::Stopped)\n> > +\t\thandleState();\n> > \n> > -\t\tstate_ = State::IpaComplete;\n> > -\t\tbreak;\n> > -\t}\n> > +\tFrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);\n> > \n> > -\tcase RPi::IPA_ACTION_EMBEDDED_COMPLETE: {\n> > -\t\tunsigned int bufferId = action.data[0];\n> > -\t\tFrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);\n> > -\t\thandleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> > -\t\tbreak;\n> > -\t}\n> > +\tLOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << bufferId\n> > +\t\t\t<< \", timestamp: \" << buffer->metadata().timestamp;\n> > \n> > -\tcase RPi::IPA_ACTION_RUN_ISP: {\n> > -\t\tunsigned int bufferId = action.data[0];\n> > -\t\tFrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);\n> > +\tisp_[Isp::Input].queueBuffer(buffer);\n> > +\tispOutputCount_ = 0;\n> > +\thandleState();\n> > +}\n> > \n> > -\t\tLOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << bufferId\n> > -\t\t\t\t<< \", timestamp: \" << buffer->metadata().timestamp;\n> > +void RPiCameraData::embeddedComplete(uint32_t bufferId)\n> > +{\n> > +\tif (state_ == State::Stopped)\n> > +\t\thandleState();\n> > \n> > -\t\tisp_[Isp::Input].queueBuffer(buffer);\n> > -\t\tispOutputCount_ = 0;\n> > -\t\tbreak;\n> > -\t}\n> > +\tFrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);\n> > +\thandleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> > +\thandleState();\n> > +}\n> > \n> > -\tdefault:\n> > -\t\tLOG(RPI, Error) << \"Unknown action \" << action.operation;\n> > -\t\tbreak;\n> > -\t}\n> > +void RPiCameraData::setIsp(const ControlList &controls)\n> > +{\n> > +\tControlList ctrls = controls;\n> > +\tisp_[Isp::Input].dev()->setControls(&ctrls);\n> > +\thandleState();\n> > +}\n> > \n> > -done:\n> > +void RPiCameraData::setStaggered(const ControlList &controls)\n> > +{\n> > +\tif (!staggeredCtrl_.set(controls))\n> > +\t\tLOG(RPI, Error) << \"V4L2 staggered set failed\";\n> > \thandleState();\n> > }\n> > \n> > @@ -1405,10 +1406,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n> > \t * application until after the IPA signals so.\n> > \t */\n> > \tif (stream == &isp_[Isp::Stats]) {\n> > -\t\tIPAOperationData op;\n> > -\t\top.operation = RPi::IPA_EVENT_SIGNAL_STAT_READY;\n> > -\t\top.data = { RPi::BufferMask::STATS | static_cast(index) };\n> > -\t\tipa_->processEvent(op);\n> > +\t\tipa_->signalStatReady(RPi::BufferMask::STATS | static_cast(index));\n> > \t} else {\n> > \t\t/* Any other ISP output can be handed back to the application now. */\n> > \t\thandleStreamBuffer(buffer, stream);\n> > @@ -1581,7 +1579,6 @@ void RPiCameraData::checkRequestCompleted()\n> > void RPiCameraData::tryRunPipeline()\n> > {\n> > \tFrameBuffer *bayerBuffer, *embeddedBuffer;\n> > -\tIPAOperationData op;\n> > \n> > \t/* If any of our request or buffer queues are empty, we cannot proceed. */\n> > \tif (state_ != State::Idle || requestQueue_.empty() ||\n> > @@ -1661,9 +1658,7 @@ void RPiCameraData::tryRunPipeline()\n> > \t * queue the ISP output buffer listed in the request to start the HW\n> > \t * pipeline.\n> > \t */\n> > -\top.operation = RPi::IPA_EVENT_QUEUE_REQUEST;\n> > -\top.controls = { request->controls() };\n> > -\tipa_->processEvent(op);\n> > +\tipa_->signalQueueRequest(request->controls());\n> > \n> > \t/* Ready to use the buffers, pop them off the queue. */\n> > \tbayerQueue_.pop();\n> > @@ -1679,10 +1674,10 @@ void RPiCameraData::tryRunPipeline()\n> > \t\t\t<< \" Bayer buffer id: \" << bayerId\n> > \t\t\t<< \" Embedded buffer id: \" << embeddedId;\n> > \n> > -\top.operation = RPi::IPA_EVENT_SIGNAL_ISP_PREPARE;\n> > -\top.data = { RPi::BufferMask::EMBEDDED_DATA | embeddedId,\n> > -\t\t RPi::BufferMask::BAYER_DATA | bayerId };\n> > -\tipa_->processEvent(op);\n> > +\tipa::rpi::IspPreparePayload ispPrepare;\n> > +\tispPrepare.embeddedbufferId_ = RPi::BufferMask::EMBEDDED_DATA | embeddedId;\n> > +\tispPrepare.bayerbufferId_ = RPi::BufferMask::BAYER_DATA | bayerId;\n> > +\tipa_->signalIspPrepare(ispPrepare);\n> > }\n> > \n> > void RPiCameraData::tryFlushQueues()","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 D13FCBE176\n\tfor ;\n\tSat, 5 Dec 2020 03:49:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5329B635F1;\n\tSat, 5 Dec 2020 04:49:22 +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 7B3EA634C6\n\tfor ;\n\tSat, 5 Dec 2020 04:49:21 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A01392A4;\n\tSat, 5 Dec 2020 04:49:19 +0100 (CET)"],"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=\"B84ZI314\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607140161;\n\tbh=AM1yzlw07HHpcoMhK2fxfZQQ7HSNRv3GoHHpcbCzapM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=B84ZI3141wcmO/ZevbiuqulXj+V/Bb/qki5VrJchOc45CMkN0VuupeBucS+mAxOvn\n\tU/EgAWRYjsuTtlUMstkwRlQsDxu3YR48SE3H3TSS986aJstudX2wfWwtJ35MM1Al/E\n\trYrQg8bAOUWGCkyfTTDM9cd6YLEMEv3ZDVTM8r3c=","Date":"Sat, 5 Dec 2020 12:49:12 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart ","Message-ID":"<20201205034912.GE2050@pyrite.rasen.tech>","References":"<20201106103707.49660-1-paul.elder@ideasonboard.com>\n\t<20201106103707.49660-27-paul.elder@ideasonboard.com>\n\t<20201126154123.GP3905@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201126154123.GP3905@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 26/37] libcamera: pipeline,\n\tipa: raspberrypi: Use new data definition","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14078,"web_url":"https://patchwork.libcamera.org/comment/14078/","msgid":"","date":"2020-12-05T14:41:08","subject":"Re: [libcamera-devel] [PATCH v4 26/37] libcamera: pipeline,\n\tipa: raspberrypi: Use new data definition","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nOn Sat, Dec 05, 2020 at 12:49:12PM +0900, paul.elder@ideasonboard.com wrote:\n> On Thu, Nov 26, 2020 at 05:41:23PM +0200, Laurent Pinchart wrote:\n> > On Fri, Nov 06, 2020 at 07:36:56PM +0900, Paul Elder wrote:\n> > > Now that we can generate custom functions and data structures with mojo,\n> > > switch the raspberrypi pipeline handler and IPA to use the custom data\n> > > structures as defined in the mojom data definition file.\n> > > \n> > > Signed-off-by: Paul Elder \n> > > \n> > > ---\n> > > Changes in v4:\n> > > - rename Controls to controls\n> > > - use the renamed header (raspberrypi_ipa_interface.h)\n> > > \n> > > Changes in v3:\n> > > - use ipa::rpi namespace\n> > > - rebase on the RPi namespace patch series newly merged into master\n> > > \n> > > Changes in v2:\n> > > - rebased on \"libcamera: pipeline: ipa: raspberrypi: Rework drop frame\n> > > signalling\"\n> > > - use newly customized RPi IPA interface\n> > > ---\n> > > include/libcamera/ipa/raspberrypi.h | 40 ++--\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 160 ++++++---------\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 193 +++++++++---------\n> > > 3 files changed, 183 insertions(+), 210 deletions(-)\n> > > \n> > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > > index df30b4a2..259f943e 100644\n> > > --- a/include/libcamera/ipa/raspberrypi.h\n> > > +++ b/include/libcamera/ipa/raspberrypi.h\n> > > @@ -28,24 +28,28 @@ enum BufferMask {\n> > > static constexpr unsigned int MaxLsGridSize = 32 << 10;\n> > > \n> > > /* List of controls handled by the Raspberry Pi IPA */\n> > > -static const ControlInfoMap Controls = {\n> > > -\t{ &controls::AeEnable, ControlInfo(false, true) },\n> > > -\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n> > > -\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> > > -\t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> > > -\t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> > > -\t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> > > -\t{ &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n> > > -\t{ &controls::AwbEnable, ControlInfo(false, true) },\n> > > -\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> > > -\t{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> > > -\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> > > -\t{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> > > -\t{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n> > > -\t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > > -\t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> > > -\t{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> > > -};\n> > > +static ControlInfoMap controls;\n> > > +\n> > > +inline void initializeRPiControls()\n> > > +{\n> > > +\tcontrols = {\n> > > +\t\t{ &controls::AeEnable, ControlInfo(false, true) },\n> > > +\t\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n> > > +\t\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> > > +\t\t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> > > +\t\t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> > > +\t\t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> > > +\t\t{ &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n> > > +\t\t{ &controls::AwbEnable, ControlInfo(false, true) },\n> > > +\t\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> > > +\t\t{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> > > +\t\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> > > +\t\t{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> > > +\t\t{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n> > > +\t\t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > > +\t\t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> > \n> > -16,+15. Looks like a mishandled rebase perhaps ? Could you double-check\n> > the values to make sure they're all fine ?\n> > \n> > > +\t};\n> > > +}\n> > \n> > Given that the IPA doesn't actually need this, I'd like to move this map\n> > to the pipeline handler. However, the generate serdes code uses this.\n> \n> The generated serdes code uses it in the event that ControlList doesn't\n> contain the ControlInfoMap. If ControlList will always have a\n> ControlInfoMap, or we can skip serdes if it doesn't, then yes, we can\n> remove the map from here.\n> \n> > Can it be dropped ? Require all pipeline handlers to define a\n> > ControlInfoMap in a shared header isn't the right way forward. Maybe\n> > with your rework of the control serdes code, following our last\n> > discussion regarding the dependency between control lists and control\n> > info maps, this will not be needed anymore ?\n> \n> Yeah. I'll look into to between v5 and v6.\n\nThank you.\n\n> > > } /* namespace RPi */\n> > > \n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index f338ff8b..316da7bd 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -19,6 +19,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > \n> > > @@ -60,7 +61,7 @@ constexpr unsigned int DefaultExposureTime = 20000;\n> > > \n> > > LOG_DEFINE_CATEGORY(IPARPI)\n> > > \n> > > -class IPARPi : public IPAInterface\n> > > +class IPARPi : public ipa::rpi::IPARPiInterface\n> > \n> > I think I commented in a previous patch that ipa::rpi::Interface may be\n> > good enough, without a need to repeat the prefix, but if I haven't, now\n> > I've expressed it :-)\n> \n> Well atm we don't require the interface definitions to declare a\n> namespace. If we do though, then we can drop the prefix, which would\n> also simplify parsing code :)\n> \n> Should we require it then?\n\nIf you think it's a good idea, I'll agree with you :-)\n\n> > > {\n> > > public:\n> > > \tIPARPi()\n> > > @@ -68,6 +69,7 @@ public:\n> > > \t\t frameCount_(0), checkCount_(0), mistrustCount_(0),\n> > > \t\t lsTable_(nullptr)\n> > > \t{\n> > > +\t\tRPi::initializeRPiControls();\n> > \n> > The IPA actually doesn't need this.\n> \n> atm this is necessary to appease the serdes, unless ControlLists are\n> guaranteed to have a ControlInfoMap.\n\nAh yes I forgot about that. I think we need to guarantee ControlLists\nhave a ControlInfoMap to be serialized.\n\n> > > \t}\n> > > \n> > > \t~IPARPi()\n> > > @@ -82,12 +84,14 @@ public:\n> > > \n> > > \tvoid configure(const CameraSensorInfo &sensorInfo,\n> > > \t\t const std::map &streamConfig,\n> > > -\t\t const std::map &entityControls,\n> > > -\t\t const IPAOperationData &data,\n> > > -\t\t IPAOperationData *response) override;\n> > > +\t\t const std::map &entityControls,\n> > \n> > Ouch, that hurts, having to make a copy of the ControlInfoMap in the\n> > caller just for the purpose of adding it to this map. Is this something\n> > that can be fixed ?\n> \n> Perhaps. We could make it so that all non-arithmetic members of\n> map/vector in function parameters are references. That should work fine,\n> right..?\n> \n> I'll give it a shot between v5 and v6.\n\nIt's certainly worth investigating. Not a blocker for merging the\nseries, but should be fixed earlier than latter if possible. Could you\nplease record it in a \\todo comment if it doesn't get address before\nmerging ?\n\n> > > +\t\t const ipa::rpi::ConfigInput &data,\n> > \n> > It feels a bit weird passing some data through dedicated parameters, and\n> > some data in a miscellaneous config structure. I'd ask if we shouldn't\n> > move everything to ipa::rpi::ConfigInput, but I fear we'll then have\n> > more of the above copy issue ?\n> \n> Well, that was just because for now I'm copying the old raspberrypi IPA\n> interface as-is.\n\nIt's also something we could address on top, it would be good to think\nabout best practice rules for IPA protocol design, and implementing them\nfor RPi as an example.\n\n> > This seems to be an annoying limitation of the IPA IPC framework. If\n> > this happens to be simple to fix (which I doubt) it would be nice to\n> > address the issue, but I assume we should do so on top. Still, if you\n> > already have ideas on how this could be handled, we can discuss it.\n> > \n> > > +\t\t ipa::rpi::ConfigOutput *response) override;\n> > > \tvoid mapBuffers(const std::vector &buffers) override;\n> > > \tvoid unmapBuffers(const std::vector &ids) override;\n> > > -\tvoid processEvent(const IPAOperationData &event) override;\n> > > +\tvoid signalStatReady(const uint32_t bufferId) override;\n> > > +\tvoid signalQueueRequest(const ControlList &controls) override;\n> > > +\tvoid signalIspPrepare(const ipa::rpi::IspPreparePayload &data) override;\n> > \n> > I like this :-)\n> > \n> > > \n> > > private:\n> > > \tvoid setMode(const CameraSensorInfo &sensorInfo);\n> > > @@ -144,6 +148,11 @@ private:\n> > > \n> > > \t/* LS table allocation passed in from the pipeline handler. */\n> > > \tFileDescriptor lsTableHandle_;\n> > > +\t/*\n> > > +\t * LS table allocation passed in from the pipeline handler,\n> > > +\t * in the context of the pipeline handler.\n> > > +\t */\n> > > +\tint32_t lsTableHandlePH_;\n> > > \tvoid *lsTable_;\n> > > };\n> > > \n> > > @@ -193,15 +202,13 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)\n> > > \n> > > void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > > \t\t [[maybe_unused]] const std::map &streamConfig,\n> > > -\t\t const std::map &entityControls,\n> > > -\t\t const IPAOperationData &ipaConfig,\n> > > -\t\t IPAOperationData *result)\n> > > +\t\t const std::map &entityControls,\n> > > +\t\t const ipa::rpi::ConfigInput &ipaConfig,\n> > > +\t\t ipa::rpi::ConfigOutput *result)\n> > > {\n> > > \tif (entityControls.empty())\n> > > \t\treturn;\n> > > \n> > > -\tresult->operation = 0;\n> > > -\n> > > \tunicamCtrls_ = entityControls.at(0);\n> > > \tispCtrls_ = entityControls.at(1);\n> > > \n> > > @@ -225,32 +232,28 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > > \t\thelper_->GetDelays(exposureDelay, gainDelay);\n> > > \t\tsensorMetadata = helper_->SensorEmbeddedDataPresent();\n> > > \n> > > -\t\tresult->data.push_back(gainDelay);\n> > > -\t\tresult->data.push_back(exposureDelay);\n> > > -\t\tresult->data.push_back(sensorMetadata);\n> > > -\n> > > -\t\tresult->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;\n> > > +\t\tresult->op_ |= ipa::rpi::RPI_IPA_CONFIG_STAGGERED_WRITE;\n> > > +\t\tresult->staggeredWriteResult_.gainDelay_ = gainDelay;\n> > > +\t\tresult->staggeredWriteResult_.exposureDelay_ = exposureDelay;\n> > > +\t\tresult->staggeredWriteResult_.sensorMetadata_ = sensorMetadata;\n> > > \t}\n> > > \n> > > \t/* Re-assemble camera mode using the sensor info. */\n> > > \tsetMode(sensorInfo);\n> > > \n> > > -\t/*\n> > > -\t * The ipaConfig.data always gives us the user transform first. Note that\n> > > -\t * this will always make the LS table pointer (if present) element 1.\n> > > -\t */\n> > > -\tmode_.transform = static_cast(ipaConfig.data[0]);\n> > > +\tmode_.transform = static_cast(ipaConfig.transform_);\n> > \n> > So much nicer :-)\n> > \n> > > \n> > > \t/* Store the lens shading table pointer and handle if available. */\n> > > -\tif (ipaConfig.operation & RPi::IPA_CONFIG_LS_TABLE) {\n> > > +\tif (ipaConfig.op_ & ipa::rpi::RPI_IPA_CONFIG_LS_TABLE) {\n> > > \t\t/* Remove any previous table, if there was one. */\n> > > \t\tif (lsTable_) {\n> > > \t\t\tmunmap(lsTable_, RPi::MaxLsGridSize);\n> > > \t\t\tlsTable_ = nullptr;\n> > > \t\t}\n> > > \n> > > -\t\t/* Map the LS table buffer into user space (now element 1). */\n> > > -\t\tlsTableHandle_ = FileDescriptor(ipaConfig.data[1]);\n> > > +\t\t/* Map the LS table buffer into user space. */\n> > > +\t\tlsTableHandle_ = FileDescriptor(ipaConfig.lsTableHandle_);\n> > > +\t\tlsTableHandlePH_ = ipaConfig.lsTableHandleStatic_;\n> > > \t\tif (lsTableHandle_.isValid()) {\n> > > \t\t\tlsTable_ = mmap(nullptr, RPi::MaxLsGridSize, PROT_READ | PROT_WRITE,\n> > > \t\t\t\t\tMAP_SHARED, lsTableHandle_.fd(), 0);\n> > > @@ -272,18 +275,15 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > > \t */\n> > > \tframeCount_ = 0;\n> > > \tcheckCount_ = 0;\n> > > -\tunsigned int dropFrame = 0;\n> > > +\tresult->op_ |= ipa::rpi::RPI_IPA_CONFIG_DROP_FRAMES;\n> > > \tif (controllerInit_) {\n> > > -\t\tdropFrame = helper_->HideFramesModeSwitch();\n> > > +\t\tresult->dropFrameCount_ = helper_->HideFramesModeSwitch();\n> > > \t\tmistrustCount_ = helper_->MistrustFramesModeSwitch();\n> > > \t} else {\n> > > -\t\tdropFrame = helper_->HideFramesStartup();\n> > > +\t\tresult->dropFrameCount_ = helper_->HideFramesStartup();\n> > > \t\tmistrustCount_ = helper_->MistrustFramesStartup();\n> > > \t}\n> > > \n> > > -\tresult->data.push_back(dropFrame);\n> > > -\tresult->operation |= RPi::IPA_CONFIG_DROP_FRAMES;\n> > > -\n> > > \t/* These zero values mean not program anything (unless overwritten). */\n> > > \tstruct AgcStatus agcStatus;\n> > > \tagcStatus.shutter_time = 0.0;\n> > > @@ -308,9 +308,9 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > > \tif (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {\n> > > \t\tControlList ctrls(unicamCtrls_);\n> > > \t\tapplyAGC(&agcStatus, ctrls);\n> > > -\t\tresult->controls.push_back(ctrls);\n> > > \n> > > -\t\tresult->operation |= RPi::IPA_CONFIG_SENSOR;\n> > > +\t\tresult->op_ |= ipa::rpi::RPI_IPA_CONFIG_SENSOR;\n> > > +\t\tresult->controls_ = ctrls;\n> > \n> > std::move() should be more efficient.\n> > \n> > > \t}\n> > > \n> > > \tlastMode_ = mode_;\n> > > @@ -348,56 +348,38 @@ void IPARPi::unmapBuffers(const std::vector &ids)\n> > > \t}\n> > > }\n> > > \n> > > -void IPARPi::processEvent(const IPAOperationData &event)\n> > > +void IPARPi::signalStatReady(const uint32_t bufferId)\n> > > {\n> > > -\tswitch (event.operation) {\n> > > -\tcase RPi::IPA_EVENT_SIGNAL_STAT_READY: {\n> > > -\t\tunsigned int bufferId = event.data[0];\n> > > -\n> > > -\t\tif (++checkCount_ != frameCount_) /* assert here? */\n> > > -\t\t\tLOG(IPARPI, Error) << \"WARNING: Prepare/Process mismatch!!!\";\n> > > -\t\tif (frameCount_ > mistrustCount_)\n> > > -\t\t\tprocessStats(bufferId);\n> > > -\n> > > -\t\treportMetadata();\n> > > -\n> > > -\t\tIPAOperationData op;\n> > > -\t\top.operation = RPi::IPA_ACTION_STATS_METADATA_COMPLETE;\n> > > -\t\top.data = { bufferId & RPi::BufferMask::ID };\n> > > -\t\top.controls = { libcameraMetadata_ };\n> > > -\t\tqueueFrameAction.emit(0, op);\n> > > -\t\tbreak;\n> > > -\t}\n> > > +\tif (++checkCount_ != frameCount_) /* assert here? */\n> > > +\t\tLOG(IPARPI, Error) << \"WARNING: Prepare/Process mismatch!!!\";\n> > > +\tif (frameCount_ > mistrustCount_)\n> > > +\t\tprocessStats(bufferId);\n> > > \n> > > -\tcase RPi::IPA_EVENT_SIGNAL_ISP_PREPARE: {\n> > > -\t\tunsigned int embeddedbufferId = event.data[0];\n> > > -\t\tunsigned int bayerbufferId = event.data[1];\n> > > +\treportMetadata();\n> > > \n> > > -\t\t/*\n> > > -\t\t * At start-up, or after a mode-switch, we may want to\n> > > -\t\t * avoid running the control algos for a few frames in case\n> > > -\t\t * they are \"unreliable\".\n> > > -\t\t */\n> > > -\t\tprepareISP(embeddedbufferId);\n> > > -\t\tframeCount_++;\n> > > -\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 & RPi::BufferMask::ID };\n> > > -\t\tqueueFrameAction.emit(0, op);\n> > > -\t\tbreak;\n> > > -\t}\n> > > +\tstatsMetadataComplete.emit(bufferId & RPi::BufferMask::ID, libcameraMetadata_);\n> > > +}\n> > > \n> > > -\tcase RPi::IPA_EVENT_QUEUE_REQUEST: {\n> > > -\t\tqueueRequest(event.controls[0]);\n> > > -\t\tbreak;\n> > > -\t}\n> > > +void IPARPi::signalQueueRequest(const ControlList &controls)\n> > > +{\n> > > +\tqueueRequest(controls);\n> > > +}\n> > > \n> > > -\tdefault:\n> > > -\t\tLOG(IPARPI, Error) << \"Unknown event \" << event.operation;\n> > > -\t\tbreak;\n> > > -\t}\n> > > +void IPARPi::signalIspPrepare(const ipa::rpi::IspPreparePayload &data)\n> > > +{\n> > > +\tunsigned int embeddedbufferId = data.embeddedbufferId_;\n> > > +\tunsigned int bayerbufferId = data.bayerbufferId_;\n> > > +\n> > > +\t/*\n> > > +\t * At start-up, or after a mode-switch, we may want to\n> > > +\t * avoid running the control algos for a few frames in case\n> > > +\t * they are \"unreliable\".\n> > > +\t */\n> > > +\tprepareISP(embeddedbufferId);\n> > > +\tframeCount_++;\n> > > +\n> > > +\t/* Ready to push the input buffer into the ISP. */\n> > > +\trunIsp.emit(bayerbufferId & RPi::BufferMask::ID);\n> > > }\n> > > \n> > > void IPARPi::reportMetadata()\n> > > @@ -498,6 +480,8 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > \t/* Clear the return metadata buffer. */\n> > > \tlibcameraMetadata_.clear();\n> > > \n> > > +\tLOG(IPARPI, Info) << \"Request ctrl length: \" << controls.size();\n> > > +\n> > \n> > Debugging leftover ?\n> > \n> > > \tfor (auto const &ctrl : controls) {\n> > > \t\tLOG(IPARPI, Info) << \"Request ctrl: \"\n> > > \t\t\t\t << controls::controls.at(ctrl.first)->name()\n> > > @@ -715,10 +699,7 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > \n> > > void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> > > {\n> > > -\tIPAOperationData op;\n> > > -\top.operation = RPi::IPA_ACTION_EMBEDDED_COMPLETE;\n> > > -\top.data = { bufferId & RPi::BufferMask::ID };\n> > > -\tqueueFrameAction.emit(0, op);\n> > > +\tembeddedComplete.emit(bufferId & RPi::BufferMask::ID);\n> > > }\n> > > \n> > > void IPARPi::prepareISP(unsigned int bufferId)\n> > > @@ -779,12 +760,8 @@ void IPARPi::prepareISP(unsigned int bufferId)\n> > > \t\tif (dpcStatus)\n> > > \t\t\tapplyDPC(dpcStatus, ctrls);\n> > > \n> > > -\t\tif (!ctrls.empty()) {\n> > > -\t\t\tIPAOperationData op;\n> > > -\t\t\top.operation = RPi::IPA_ACTION_V4L2_SET_ISP;\n> > > -\t\t\top.controls.push_back(ctrls);\n> > > -\t\t\tqueueFrameAction.emit(0, op);\n> > > -\t\t}\n> > > +\t\tif (!ctrls.empty())\n> > > +\t\t\tsetIsp.emit(ctrls);\n> > > \t}\n> > > }\n> > > \n> > > @@ -840,10 +817,7 @@ void IPARPi::processStats(unsigned int bufferId)\n> > > \t\tControlList ctrls(unicamCtrls_);\n> > > \t\tapplyAGC(&agcStatus, ctrls);\n> > > \n> > > -\t\tIPAOperationData op;\n> > > -\t\top.operation = RPi::IPA_ACTION_V4L2_SET_STAGGERED;\n> > > -\t\top.controls.push_back(ctrls);\n> > > -\t\tqueueFrameAction.emit(0, op);\n> > > +\t\tsetStaggered.emit(ctrls);\n> > > \t}\n> > > }\n> > > \n> > > @@ -1073,7 +1047,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> > > \t\t.grid_width = w,\n> > > \t\t.grid_stride = w,\n> > > \t\t.grid_height = h,\n> > > -\t\t.dmabuf = lsTableHandle_.fd(),\n> > > +\t\t.dmabuf = lsTableHandlePH_,\n> > \n> > This feels like a bit of a hack. How about simplifying it, by dropping\n> > lsTableHandlePH_ here and filling the field in the pipeline handler\n> > instead ?\n> \n> This is also what the raspberrypi IPA was doing before.\n\nRPi doesn't care right now because there's no isolation, so the original\nfile descriptor number doesn't need to be preserved separately. Also\nfixable on top (with a \\todo) if needed.\n\n> > > \t\t.ref_transform = 0,\n> > > \t\t.corner_sampled = 1,\n> > > \t\t.gain_format = GAIN_FORMAT_U4P10\n> > > @@ -1150,9 +1124,9 @@ const struct IPAModuleInfo ipaModuleInfo = {\n> > > \t\"raspberrypi\",\n> > > };\n> > > \n> > > -struct ipa_context *ipaCreate()\n> > > +IPAInterface *ipaCreate()\n> > > {\n> > > -\treturn new IPAInterfaceWrapper(std::make_unique());\n> > > +\treturn new IPARPi();\n> > > }\n> > > \n> > > } /* extern \"C\" */\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 0a60442c..0e9ff16d 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -16,7 +16,9 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -144,7 +146,11 @@ public:\n> > > \tint loadIPA();\n> > > \tint configureIPA(const CameraConfiguration *config);\n> > > \n> > > -\tvoid queueFrameAction(unsigned int frame, const IPAOperationData &action);\n> > > +\tvoid statsMetadataComplete(uint32_t bufferId, const ControlList &controls);\n> > > +\tvoid runIsp(uint32_t bufferId);\n> > > +\tvoid embeddedComplete(uint32_t bufferId);\n> > > +\tvoid setIsp(const ControlList &controls);\n> > > +\tvoid setStaggered(const ControlList &controls);\n> > > \n> > > \t/* bufferComplete signal handlers. */\n> > > \tvoid unicamBufferDequeue(FrameBuffer *buffer);\n> > > @@ -156,6 +162,8 @@ public:\n> > > \tvoid handleExternalBuffer(FrameBuffer *buffer, RPi::Stream *stream);\n> > > \tvoid handleState();\n> > > \n> > > +\tstd::unique_ptr ipa_;\n> > > +\n> > > \tCameraSensor *sensor_;\n> > > \t/* Array of Unicam and ISP device streams and associated buffers/streams. */\n> > > \tRPi::Device unicam_;\n> > > @@ -448,6 +456,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > > PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)\n> > > \t: PipelineHandler(manager), unicam_(nullptr), isp_(nullptr)\n> > > {\n> > > +\tRPi::initializeRPiControls();\n> > > }\n> > > \n> > > CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > > @@ -936,7 +945,7 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > \t}\n> > > \n> > > \t/* Register the controls that the Raspberry Pi IPA can handle. */\n> > > -\tdata->controlInfo_ = RPi::Controls;\n> > > +\tdata->controlInfo_ = RPi::controls;\n> > > \t/* Initialize the camera properties. */\n> > > \tdata->properties_ = data->sensor_->properties();\n> > > \n> > > @@ -1114,11 +1123,16 @@ void RPiCameraData::frameStarted(uint32_t sequence)\n> > > \n> > > int RPiCameraData::loadIPA()\n> > > {\n> > > -\tipa_ = IPAManager::createIPA(pipe_, 1, 1);\n> > > +\tipa_ = IPAManager::createIPA(pipe_, 1, 1);\n> > > +\n> > > \tif (!ipa_)\n> > > \t\treturn -ENOENT;\n> > > \n> > > -\tipa_->queueFrameAction.connect(this, &RPiCameraData::queueFrameAction);\n> > > +\tipa_->statsMetadataComplete.connect(this, &RPiCameraData::statsMetadataComplete);\n> > > +\tipa_->runIsp.connect(this, &RPiCameraData::runIsp);\n> > > +\tipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete);\n> > > +\tipa_->setIsp.connect(this, &RPiCameraData::setIsp);\n> > > +\tipa_->setStaggered.connect(this, &RPiCameraData::setStaggered);\n> > > \n> > > \tIPASettings settings{\n> > > \t\t.configurationFile = ipa_->configurationFile(sensor_->model() + \".json\")\n> > > @@ -1134,8 +1148,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > > \t\tstatic_cast(config);\n> > > \n> > > \tstd::map streamConfig;\n> > > -\tstd::map entityControls;\n> > > -\tIPAOperationData ipaConfig = {};\n> > > +\tstd::map entityControls;\n> > > +\tipa::rpi::ConfigInput ipaConfig;\n> > > \n> > > \t/* Get the device format to pass to the IPA. */\n> > > \tV4L2DeviceFormat sensorFormat;\n> > > @@ -1155,7 +1169,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > > \tentityControls.emplace(1, isp_[Isp::Input].dev()->controls());\n> > > \n> > > \t/* Always send the user transform to the IPA. */\n> > > -\tipaConfig.data = { static_cast(config->transform) };\n> > > +\tipaConfig.transform_ = static_cast(config->transform);\n> > > \n> > > \t/* Allocate the lens shading table via dmaHeap and pass to the IPA. */\n> > > \tif (!lsTable_.isValid()) {\n> > > @@ -1164,8 +1178,9 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > > \t\t\treturn -ENOMEM;\n> > > \n> > > \t\t/* Allow the IPA to mmap the LS table via the file descriptor. */\n> > > -\t\tipaConfig.operation = RPi::IPA_CONFIG_LS_TABLE;\n> > > -\t\tipaConfig.data.push_back(static_cast(lsTable_.fd()));\n> > > +\t\tipaConfig.op_ |= ipa::rpi::RPI_IPA_CONFIG_LS_TABLE;\n> > > +\t\tipaConfig.lsTableHandle_ = lsTable_;\n> > > +\t\tipaConfig.lsTableHandleStatic_ = lsTable_.fd();\n> > > \t}\n> > > \n> > > \t/* We store the CameraSensorInfo for digital zoom calculations. */\n> > > @@ -1176,34 +1191,35 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > > \t}\n> > > \n> > > \t/* Ready the IPA - it must know about the sensor resolution. */\n> > > -\tIPAOperationData result;\n> > > +\tipa::rpi::ConfigOutput result;\n> > > \n> > > \tipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,\n> > > \t\t\t&result);\n> > > \n> > > -\tunsigned int resultIdx = 0;\n> > > -\tif (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {\n> > > +\tif (result.op_ & ipa::rpi::RPI_IPA_CONFIG_STAGGERED_WRITE) {\n> > > \t\t/*\n> > > \t\t * Setup our staggered control writer with the sensor default\n> > > \t\t * gain and exposure delays.\n> > > \t\t */\n> > > \t\tif (!staggeredCtrl_) {\n> > > \t\t\tstaggeredCtrl_.init(unicam_[Unicam::Image].dev(),\n> > > -\t\t\t\t\t { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },\n> > > -\t\t\t\t\t { V4L2_CID_EXPOSURE, result.data[resultIdx++] } });\n> > > -\t\t\tsensorMetadata_ = result.data[resultIdx++];\n> > > +\t\t\t\t\t{ { V4L2_CID_ANALOGUE_GAIN,\n> > > +\t\t\t\t\tresult.staggeredWriteResult_.gainDelay_ },\n> > > +\t\t\t\t\t{ V4L2_CID_EXPOSURE,\n> > > +\t\t\t\t\tresult.staggeredWriteResult_.exposureDelay_ } });\n> > > +\t\t\tsensorMetadata_ = result.staggeredWriteResult_.sensorMetadata_;\n> > > \t\t}\n> > > \t}\n> > > \n> > > -\tif (result.operation & RPi::IPA_CONFIG_SENSOR) {\n> > > -\t\tconst ControlList &ctrls = result.controls[0];\n> > > +\tif (result.op_ & ipa::rpi::RPI_IPA_CONFIG_SENSOR) {\n> > > +\t\tconst ControlList &ctrls = result.controls_;\n> > > \t\tif (!staggeredCtrl_.set(ctrls))\n> > > \t\t\tLOG(RPI, Error) << \"V4L2 staggered set failed\";\n> > > \t}\n> > > \n> > > -\tif (result.operation & RPi::IPA_CONFIG_DROP_FRAMES) {\n> > > +\tif (result.op_ & ipa::rpi::RPI_IPA_CONFIG_DROP_FRAMES) {\n> > > \t\t/* Configure the number of dropped frames required on startup. */\n> > > -\t\tdropFrameCount_ = result.data[resultIdx++];\n> > > +\t\tdropFrameCount_ = result.dropFrameCount_;\n> > > \t}\n> > > \n> > > \t/*\n> > > @@ -1222,90 +1238,75 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > > \treturn 0;\n> > > }\n> > > \n> > > -void RPiCameraData::queueFrameAction([[maybe_unused]] unsigned int frame,\n> > > -\t\t\t\t const IPAOperationData &action)\n> > > +void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)\n> > > {\n> > > -\t/*\n> > > -\t * The following actions can be handled when the pipeline handler is in\n> > > -\t * a stopped state.\n> > > -\t */\n> > > -\tswitch (action.operation) {\n> > > -\tcase RPi::IPA_ACTION_V4L2_SET_STAGGERED: {\n> > > -\t\tconst ControlList &controls = action.controls[0];\n> > > -\t\tif (!staggeredCtrl_.set(controls))\n> > > -\t\t\tLOG(RPI, Error) << \"V4L2 staggered set failed\";\n> > > -\t\tgoto done;\n> > > -\t}\n> > > +\tif (state_ == State::Stopped)\n> > > +\t\thandleState();\n> > > \n> > > -\tcase RPi::IPA_ACTION_V4L2_SET_ISP: {\n> > > -\t\tControlList controls = action.controls[0];\n> > > -\t\tisp_[Isp::Input].dev()->setControls(&controls);\n> > > -\t\tgoto done;\n> > > -\t}\n> > > -\t}\n> > > +\tFrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> > > \n> > > -\tif (state_ == State::Stopped)\n> > > -\t\tgoto done;\n> > > +\thandleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> > > +\n> > > +\t/* Fill the Request metadata buffer with what the IPA has provided */\n> > > +\tRequest *request = requestQueue_.front();\n> > > +\trequest->metadata() = std::move(controls);\n> > > \n> > > \t/*\n> > > -\t * The following actions must not be handled when the pipeline handler\n> > > -\t * is in a stopped state.\n> > > +\t * Also update the ScalerCrop in the metadata with what we actually\n> > > +\t * used. But we must first rescale that from ISP (camera mode) pixels\n> > > +\t * back into sensor native pixels.\n> > > +\t *\n> > > +\t * Sending this information on every frame may be helpful.\n> > > \t */\n> > > -\tswitch (action.operation) {\n> > > -\tcase RPi::IPA_ACTION_STATS_METADATA_COMPLETE: {\n> > > -\t\tunsigned int bufferId = action.data[0];\n> > > -\t\tFrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> > > +\tif (updateScalerCrop_) {\n> > > +\t\tupdateScalerCrop_ = false;\n> > > +\t\tscalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> > > +\t\t\t\tsensorInfo_.outputSize);\n> > > +\t\tscalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n> > > +\t}\n> > > +\trequest->metadata().set(controls::ScalerCrop, scalerCrop_);\n> > > \n> > > -\t\thandleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> > > +\tstate_ = State::IpaComplete;\n> > > \n> > > -\t\t/* Fill the Request metadata buffer with what the IPA has provided */\n> > > -\t\tRequest *request = requestQueue_.front();\n> > > -\t\trequest->metadata() = std::move(action.controls[0]);\n> > > +\thandleState();\n> > > +}\n> > > \n> > > -\t\t/*\n> > > -\t\t * Also update the ScalerCrop in the metadata with what we actually\n> > > -\t\t * used. But we must first rescale that from ISP (camera mode) pixels\n> > > -\t\t * back into sensor native pixels.\n> > > -\t\t *\n> > > -\t\t * Sending this information on every frame may be helpful.\n> > > -\t\t */\n> > > -\t\tif (updateScalerCrop_) {\n> > > -\t\t\tupdateScalerCrop_ = false;\n> > > -\t\t\tscalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> > > -\t\t\t\t\t\t\tsensorInfo_.outputSize);\n> > > -\t\t\tscalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n> > > -\t\t}\n> > > -\t\trequest->metadata().set(controls::ScalerCrop, scalerCrop_);\n> > > +void RPiCameraData::runIsp(uint32_t bufferId)\n> > > +{\n> > > +\tif (state_ == State::Stopped)\n> > > +\t\thandleState();\n> > > \n> > > -\t\tstate_ = State::IpaComplete;\n> > > -\t\tbreak;\n> > > -\t}\n> > > +\tFrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);\n> > > \n> > > -\tcase RPi::IPA_ACTION_EMBEDDED_COMPLETE: {\n> > > -\t\tunsigned int bufferId = action.data[0];\n> > > -\t\tFrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);\n> > > -\t\thandleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> > > -\t\tbreak;\n> > > -\t}\n> > > +\tLOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << bufferId\n> > > +\t\t\t<< \", timestamp: \" << buffer->metadata().timestamp;\n> > > \n> > > -\tcase RPi::IPA_ACTION_RUN_ISP: {\n> > > -\t\tunsigned int bufferId = action.data[0];\n> > > -\t\tFrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);\n> > > +\tisp_[Isp::Input].queueBuffer(buffer);\n> > > +\tispOutputCount_ = 0;\n> > > +\thandleState();\n> > > +}\n> > > \n> > > -\t\tLOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << bufferId\n> > > -\t\t\t\t<< \", timestamp: \" << buffer->metadata().timestamp;\n> > > +void RPiCameraData::embeddedComplete(uint32_t bufferId)\n> > > +{\n> > > +\tif (state_ == State::Stopped)\n> > > +\t\thandleState();\n> > > \n> > > -\t\tisp_[Isp::Input].queueBuffer(buffer);\n> > > -\t\tispOutputCount_ = 0;\n> > > -\t\tbreak;\n> > > -\t}\n> > > +\tFrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);\n> > > +\thandleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> > > +\thandleState();\n> > > +}\n> > > \n> > > -\tdefault:\n> > > -\t\tLOG(RPI, Error) << \"Unknown action \" << action.operation;\n> > > -\t\tbreak;\n> > > -\t}\n> > > +void RPiCameraData::setIsp(const ControlList &controls)\n> > > +{\n> > > +\tControlList ctrls = controls;\n> > > +\tisp_[Isp::Input].dev()->setControls(&ctrls);\n> > > +\thandleState();\n> > > +}\n> > > \n> > > -done:\n> > > +void RPiCameraData::setStaggered(const ControlList &controls)\n> > > +{\n> > > +\tif (!staggeredCtrl_.set(controls))\n> > > +\t\tLOG(RPI, Error) << \"V4L2 staggered set failed\";\n> > > \thandleState();\n> > > }\n> > > \n> > > @@ -1405,10 +1406,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n> > > \t * application until after the IPA signals so.\n> > > \t */\n> > > \tif (stream == &isp_[Isp::Stats]) {\n> > > -\t\tIPAOperationData op;\n> > > -\t\top.operation = RPi::IPA_EVENT_SIGNAL_STAT_READY;\n> > > -\t\top.data = { RPi::BufferMask::STATS | static_cast(index) };\n> > > -\t\tipa_->processEvent(op);\n> > > +\t\tipa_->signalStatReady(RPi::BufferMask::STATS | static_cast(index));\n> > > \t} else {\n> > > \t\t/* Any other ISP output can be handed back to the application now. */\n> > > \t\thandleStreamBuffer(buffer, stream);\n> > > @@ -1581,7 +1579,6 @@ void RPiCameraData::checkRequestCompleted()\n> > > void RPiCameraData::tryRunPipeline()\n> > > {\n> > > \tFrameBuffer *bayerBuffer, *embeddedBuffer;\n> > > -\tIPAOperationData op;\n> > > \n> > > \t/* If any of our request or buffer queues are empty, we cannot proceed. */\n> > > \tif (state_ != State::Idle || requestQueue_.empty() ||\n> > > @@ -1661,9 +1658,7 @@ void RPiCameraData::tryRunPipeline()\n> > > \t * queue the ISP output buffer listed in the request to start the HW\n> > > \t * pipeline.\n> > > \t */\n> > > -\top.operation = RPi::IPA_EVENT_QUEUE_REQUEST;\n> > > -\top.controls = { request->controls() };\n> > > -\tipa_->processEvent(op);\n> > > +\tipa_->signalQueueRequest(request->controls());\n> > > \n> > > \t/* Ready to use the buffers, pop them off the queue. */\n> > > \tbayerQueue_.pop();\n> > > @@ -1679,10 +1674,10 @@ void RPiCameraData::tryRunPipeline()\n> > > \t\t\t<< \" Bayer buffer id: \" << bayerId\n> > > \t\t\t<< \" Embedded buffer id: \" << embeddedId;\n> > > \n> > > -\top.operation = RPi::IPA_EVENT_SIGNAL_ISP_PREPARE;\n> > > -\top.data = { RPi::BufferMask::EMBEDDED_DATA | embeddedId,\n> > > -\t\t RPi::BufferMask::BAYER_DATA | bayerId };\n> > > -\tipa_->processEvent(op);\n> > > +\tipa::rpi::IspPreparePayload ispPrepare;\n> > > +\tispPrepare.embeddedbufferId_ = RPi::BufferMask::EMBEDDED_DATA | embeddedId;\n> > > +\tispPrepare.bayerbufferId_ = RPi::BufferMask::BAYER_DATA | bayerId;\n> > > +\tipa_->signalIspPrepare(ispPrepare);\n> > > }\n> > > \n> > > void RPiCameraData::tryFlushQueues()","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 D5662BDB20\n\tfor ;\n\tSat, 5 Dec 2020 14:41:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 61F7A63607;\n\tSat, 5 Dec 2020 15:41:11 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4AFF7635F0\n\tfor ;\n\tSat, 5 Dec 2020 15:41:10 +0100 (CET)","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 311FEB95;\n\tSat, 5 Dec 2020 15:41:09 +0100 (CET)"],"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=\"CP1/JDvF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607179269;\n\tbh=v/EewvjAzqwquACn6hUw0ZYuM6s+vTHY33UOtk1MTs0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CP1/JDvFdPjXNWWAteKpBpDf7MeIv5TQfPfaQMNY8dgnjKreG56atqOcFQ267uKb7\n\tPDnhIwkCAtf1Gn0rjKi7/eJhugkMG9H7PRBsEB1rn3or6JrDibVU2tYoWrkxHCJKzX\n\t12g/DM6KwG0BHZWEiZKqzZAY7+rCHUY2Ilune7Lg=","Date":"Sat, 5 Dec 2020 16:41:08 +0200","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20201106103707.49660-1-paul.elder@ideasonboard.com>\n\t<20201106103707.49660-27-paul.elder@ideasonboard.com>\n\t<20201126154123.GP3905@pendragon.ideasonboard.com>\n\t<20201205034912.GE2050@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201205034912.GE2050@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v4 26/37] libcamera: pipeline,\n\tipa: raspberrypi: Use new data definition","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]