[{"id":12587,"web_url":"https://patchwork.libcamera.org/comment/12587/","msgid":"<20200919124234.GT1850958@oden.dyn.berto.se>","date":"2020-09-19T12:42:34","subject":"Re: [libcamera-devel] [PATCH 18/23] libcamera: pipeline,\n\tipa: raspberrypi: Use new data definition","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Paul,\n\nThanks for your work.\n\nOn 2020-09-15 23:20:33 +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 <paul.elder@ideasonboard.com>\n> ---\n>  include/libcamera/ipa/raspberrypi.h           |  37 +++---\n>  src/ipa/raspberrypi/raspberrypi.cpp           | 108 +++++++++-------\n>  .../pipeline/raspberrypi/raspberrypi.cpp      | 120 +++++++++++-------\n>  3 files changed, 155 insertions(+), 110 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index ed2b12d5..e58d1cbc 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -23,22 +23,27 @@ enum RPiIpaMask {\n>  namespace libcamera {\n>  \n>  /* List of controls handled by the Raspberry Pi IPA */\n> -static const ControlInfoMap RPiControls = {\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(0, static_cast<int32_t>(controls::MeteringModeMax)) },\n> -\t{ &controls::AeConstraintMode, ControlInfo(0, static_cast<int32_t>(controls::ConstraintModeMax)) },\n> -\t{ &controls::AeExposureMode, ControlInfo(0, static_cast<int32_t>(controls::ExposureModeMax)) },\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(0, static_cast<int32_t>(controls::AwbModeMax)) },\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> +static ControlInfoMap RPiControls;\n> +\n> +inline void initializeRPiControls()\n> +{\n> +\tRPiControls = {\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(0, static_cast<int32_t>(controls::MeteringModeMax)) },\n> +\t\t{ &controls::AeConstraintMode, ControlInfo(0, static_cast<int32_t>(controls::ConstraintModeMax)) },\n> +\t\t{ &controls::AeExposureMode, ControlInfo(0, static_cast<int32_t>(controls::ExposureModeMax)) },\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(0, static_cast<int32_t>(controls::AwbModeMax)) },\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> +\t};\n\nWhy do you nee to move this to a function?\n\n>  };\n>  \n>  } /* namespace libcamera */\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 4557016c..e09caa8d 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -19,6 +19,7 @@\n>  #include <libcamera/ipa/ipa_interface.h>\n>  #include <libcamera/ipa/ipa_module_info.h>\n>  #include <libcamera/ipa/raspberrypi.h>\n> +#include <libcamera/ipa/raspberrypi_generated.h>\n>  #include <libcamera/request.h>\n>  #include <libcamera/span.h>\n>  \n> @@ -60,7 +61,7 @@ namespace libcamera {\n>  \n>  LOG_DEFINE_CATEGORY(IPARPI)\n>  \n> -class IPARPi : public IPAInterface\n> +class IPARPi : public IPARPiInterface\n>  {\n>  public:\n>  \tIPARPi()\n> @@ -68,6 +69,7 @@ public:\n>  \t\t  frame_count_(0), check_count_(0), hide_count_(0),\n>  \t\t  mistrust_count_(0), lsTable_(nullptr)\n>  \t{\n> +\t\tinitializeRPiControls();\n>  \t}\n>  \n>  \t~IPARPi()\n> @@ -82,12 +84,12 @@ public:\n>  \n>  \tvoid configure(const CameraSensorInfo &sensorInfo,\n>  \t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n> -\t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls,\n> -\t\t       const IPAOperationData &data,\n> -\t\t       IPAOperationData *response) override;\n> +\t\t       const std::map<unsigned int, ControlInfoMap> &entityControls,\n\nIs it by design or a effect/limitation of mojo we can't keep const & ?\n\n> +\t\t       const RPiConfigureParams &data,\n> +\t\t       RPiConfigureParams *response) override;\n>  \tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override;\n>  \tvoid unmapBuffers(const std::vector<unsigned int> &ids) override;\n> -\tvoid processEvent(const IPAOperationData &event) override;\n> +\tvoid processEvent(const RPiEventParams &event) override;\n>  \n>  private:\n>  \tvoid setMode(const CameraSensorInfo &sensorInfo);\n> @@ -143,6 +145,11 @@ private:\n>  \tunsigned int mistrust_count_;\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> @@ -192,15 +199,13 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)\n>  \n>  void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n>  \t\t       [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,\n> -\t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls,\n> -\t\t       const IPAOperationData &ipaConfig,\n> -\t\t       IPAOperationData *result)\n> +\t\t       const std::map<unsigned int, ControlInfoMap> &entityControls,\n> +\t\t       const RPiConfigureParams &ipaConfig,\n> +\t\t       RPiConfigureParams *result)\n>  {\n>  \tif (entityControls.empty())\n>  \t\treturn;\n>  \n> -\tresult->operation = 0;\n> -\n>  \tunicam_ctrls_ = entityControls.at(0);\n>  \tisp_ctrls_ = entityControls.at(1);\n>  \t/* Setup a metadata ControlList to output metadata. */\n> @@ -222,11 +227,13 @@ 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> +\t\tRPiConfigurePayload payload = {};\n> +\t\tpayload.op_ = RPI_IPA_CONFIG_STAGGERED_WRITE;\n> +\t\tpayload.staggeredWriteResult_.gainDelay_ = gainDelay;\n> +\t\tpayload.staggeredWriteResult_.exposureDelay_ = exposureDelay;\n> +\t\tpayload.staggeredWriteResult_.sensorMetadata_ = sensorMetadata;\n\nSo much nicer, I really like this!\n\n>  \n> -\t\tresult->operation |= RPI_IPA_CONFIG_STAGGERED_WRITE;\n> +\t\tresult->payload_.push_back(payload);\n>  \t}\n>  \n>  \t/* Re-assemble camera mode using the sensor info. */\n> @@ -274,15 +281,23 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n>  \tif (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {\n>  \t\tControlList ctrls(unicam_ctrls_);\n>  \t\tapplyAGC(&agcStatus, ctrls);\n> -\t\tresult->controls.push_back(ctrls);\n>  \n> -\t\tresult->operation |= RPI_IPA_CONFIG_SENSOR;\n> +\t\tRPiConfigurePayload payload = {};\n> +\t\tpayload.op_ = RPI_IPA_CONFIG_SENSOR;\n> +\t\tpayload.controls_ = ctrls;\n> +\n> +\t\tresult->payload_.push_back(payload);\n>  \t}\n>  \n>  \tlastMode_ = mode_;\n>  \n>  \t/* Store the lens shading table pointer and handle if available. */\n> -\tif (ipaConfig.operation & RPI_IPA_CONFIG_LS_TABLE) {\n> +\tauto lens = std::find_if(ipaConfig.payload_.begin(),\n> +\t\t\t\t ipaConfig.payload_.end(),\n> +\t\t\t\t [] (const RPiConfigurePayload &p) {\n> +\t\t\t\t\t return p.op_ == RPI_IPA_CONFIG_LS_TABLE;\n> +\t\t\t\t });\n\nI think this warrents a helper, is it possible to have a base class for \nRPiConfigureParams which could be shared between pipelines to have such \nhelpers or do we think that is to restricitve and should do so for each \nIPA protocol implementation?\n\n> +\tif (lens != ipaConfig.payload_.end()) {\n>  \t\t/* Remove any previous table, if there was one. */\n>  \t\tif (lsTable_) {\n>  \t\t\tmunmap(lsTable_, MAX_LS_GRID_SIZE);\n> @@ -290,7 +305,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n>  \t\t}\n>  \n>  \t\t/* Map the LS table buffer into user space. */\n> -\t\tlsTableHandle_ = FileDescriptor(ipaConfig.data[0]);\n> +\t\tlsTableHandle_ = FileDescriptor(lens->lsTableHandle_);\n> +\t\tlsTableHandlePH_ = lens->lsTableHandleStatic_;\n>  \t\tif (lsTableHandle_.isValid()) {\n>  \t\t\tlsTable_ = mmap(nullptr, MAX_LS_GRID_SIZE, PROT_READ | PROT_WRITE,\n>  \t\t\t\t\tMAP_SHARED, lsTableHandle_.fd(), 0);\n> @@ -335,11 +351,11 @@ void IPARPi::unmapBuffers(const std::vector<unsigned int> &ids)\n>  \t}\n>  }\n>  \n> -void IPARPi::processEvent(const IPAOperationData &event)\n> +void IPARPi::processEvent(const RPiEventParams &event)\n>  {\n> -\tswitch (event.operation) {\n> +\tswitch (event.ev_) {\n>  \tcase RPI_IPA_EVENT_SIGNAL_STAT_READY: {\n> -\t\tunsigned int bufferId = event.data[0];\n> +\t\tunsigned int bufferId = event.bufferId_;\n>  \n>  \t\tif (++check_count_ != frame_count_) /* assert here? */\n>  \t\t\tLOG(IPARPI, Error) << \"WARNING: Prepare/Process mismatch!!!\";\n> @@ -348,17 +364,17 @@ void IPARPi::processEvent(const IPAOperationData &event)\n>  \n>  \t\treportMetadata();\n>  \n> -\t\tIPAOperationData op;\n> -\t\top.operation = RPI_IPA_ACTION_STATS_METADATA_COMPLETE;\n> -\t\top.data = { bufferId & RPiIpaMask::ID };\n> -\t\top.controls = { libcameraMetadata_ };\n> +\t\tRPiActionParams op;\n> +\t\top.op_ = RPI_IPA_ACTION_STATS_METADATA_COMPLETE;\n> +\t\top.statsComplete_.bufferId_ = { bufferId & RPiIpaMask::ID };\n> +\t\top.statsComplete_.controls_ = { libcameraMetadata_ };\n>  \t\tqueueFrameAction.emit(0, op);\n>  \t\tbreak;\n>  \t}\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> +\t\tunsigned int embeddedbufferId = event.ispPrepare_.embeddedbufferId_;\n> +\t\tunsigned int bayerbufferId = event.ispPrepare_.bayerbufferId_;\n>  \n>  \t\t/*\n>  \t\t * At start-up, or after a mode-switch, we may want to\n> @@ -368,23 +384,23 @@ void IPARPi::processEvent(const IPAOperationData &event)\n>  \t\tprepareISP(embeddedbufferId);\n>  \n>  \t\t/* Ready to push the input buffer into the ISP. */\n> -\t\tIPAOperationData op;\n> +\t\tRPiActionParams op;\n>  \t\tif (++frame_count_ > hide_count_)\n> -\t\t\top.operation = RPI_IPA_ACTION_RUN_ISP;\n> +\t\t\top.op_ = RPI_IPA_ACTION_RUN_ISP;\n>  \t\telse\n> -\t\t\top.operation = RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME;\n> -\t\top.data = { bayerbufferId & RPiIpaMask::ID };\n> +\t\t\top.op_ = RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME;\n> +\t\top.bufferId_ = { bayerbufferId & RPiIpaMask::ID };\n>  \t\tqueueFrameAction.emit(0, op);\n>  \t\tbreak;\n>  \t}\n>  \n>  \tcase RPI_IPA_EVENT_QUEUE_REQUEST: {\n> -\t\tqueueRequest(event.controls[0]);\n> +\t\tqueueRequest(event.controls_);\n>  \t\tbreak;\n>  \t}\n>  \n>  \tdefault:\n> -\t\tLOG(IPARPI, Error) << \"Unknown event \" << event.operation;\n> +\t\tLOG(IPARPI, Error) << \"Unknown event \" << event.ev_;\n>  \t\tbreak;\n>  \t}\n>  }\n> @@ -489,6 +505,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>  \tfor (auto const &ctrl : controls) {\n>  \t\tLOG(IPARPI, Info) << \"Request ctrl: \"\n>  \t\t\t\t  << controls::controls.at(ctrl.first)->name()\n> @@ -698,9 +716,9 @@ 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 & RPiIpaMask::ID };\n> +\tRPiActionParams op;\n> +\top.op_ = RPI_IPA_ACTION_EMBEDDED_COMPLETE;\n> +\top.bufferId_ = { bufferId & RPiIpaMask::ID };\n>  \tqueueFrameAction.emit(0, op);\n>  }\n>  \n> @@ -763,9 +781,9 @@ void IPARPi::prepareISP(unsigned int bufferId)\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\tRPiActionParams op;\n> +\t\t\top.op_ = RPI_IPA_ACTION_V4L2_SET_ISP;\n> +\t\t\top.controls_ = ctrls;\n>  \t\t\tqueueFrameAction.emit(0, op);\n>  \t\t}\n>  \t}\n> @@ -823,9 +841,9 @@ void IPARPi::processStats(unsigned int bufferId)\n>  \t\tControlList ctrls(unicam_ctrls_);\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\tRPiActionParams op;\n> +\t\top.op_ = RPI_IPA_ACTION_V4L2_SET_STAGGERED;\n> +\t\top.controls_ = ctrls;\n>  \t\tqueueFrameAction.emit(0, op);\n>  \t}\n>  }\n> @@ -1056,7 +1074,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>  \t\t.ref_transform = 0,\n>  \t\t.corner_sampled = 1,\n>  \t\t.gain_format = GAIN_FORMAT_U4P10\n> @@ -1136,9 +1154,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<IPARPi>());\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 ce43af34..70bb6fcc 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -15,7 +15,9 @@\n>  #include <libcamera/control_ids.h>\n>  #include <libcamera/file_descriptor.h>\n>  #include <libcamera/formats.h>\n> +#include <libcamera/ipa/ipa_proxy_raspberrypi.h>\n>  #include <libcamera/ipa/raspberrypi.h>\n> +#include <libcamera/ipa/raspberrypi_generated.h>\n>  #include <libcamera/logging.h>\n>  #include <libcamera/property_ids.h>\n>  #include <libcamera/request.h>\n> @@ -296,7 +298,7 @@ public:\n>  \tint loadIPA();\n>  \tint configureIPA();\n>  \n> -\tvoid queueFrameAction(unsigned int frame, const IPAOperationData &action);\n> +\tvoid queueFrameAction(unsigned int frame, const RPiActionParams &action);\n>  \n>  \t/* bufferComplete signal handlers. */\n>  \tvoid unicamBufferDequeue(FrameBuffer *buffer);\n> @@ -307,6 +309,8 @@ public:\n>  \tvoid handleStreamBuffer(FrameBuffer *buffer, const RPiStream *stream);\n>  \tvoid handleState();\n>  \n> +\tstd::unique_ptr<IPAProxyRPi> ipa_;\n> +\n>  \tCameraSensor *sensor_;\n>  \t/* Array of Unicam and ISP device streams and associated buffers/streams. */\n>  \tRPiDevice<Unicam, 2> unicam_;\n> @@ -506,6 +510,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n>  PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)\n>  \t: PipelineHandler(manager), unicam_(nullptr), isp_(nullptr)\n>  {\n> +\tinitializeRPiControls();\n>  }\n>  \n>  CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> @@ -1103,7 +1108,9 @@ void RPiCameraData::frameStarted(uint32_t sequence)\n>  \n>  int RPiCameraData::loadIPA()\n>  {\n> -\tipa_ = IPAManager::createIPA(pipe_, 1, 1);\n> +\tstd::unique_ptr<IPAProxy> ptr = IPAManager::createIPA(pipe_, 1, 1);\n> +\tipa_ = std::unique_ptr<IPAProxyRPi>{static_cast<IPAProxyRPi*>(std::move(ptr).release())};\n> +\n>  \tif (!ipa_)\n>  \t\treturn -ENOENT;\n>  \n> @@ -1119,8 +1126,8 @@ int RPiCameraData::loadIPA()\n>  int RPiCameraData::configureIPA()\n>  {\n>  \tstd::map<unsigned int, IPAStream> streamConfig;\n> -\tstd::map<unsigned int, const ControlInfoMap &> entityControls;\n> -\tIPAOperationData ipaConfig = {};\n> +\tstd::map<unsigned int, ControlInfoMap> entityControls;\n> +\tRPiConfigureParams ipaConfig;\n>  \n>  \t/* Get the device format to pass to the IPA. */\n>  \tV4L2DeviceFormat sensorFormat;\n> @@ -1145,8 +1152,11 @@ int RPiCameraData::configureIPA()\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 = { static_cast<unsigned int>(lsTable_.fd()) };\n> +\t\tRPiConfigurePayload payload;\n> +\t\tpayload.op_ = RPI_IPA_CONFIG_LS_TABLE;\n> +\t\tpayload.lsTableHandle_ = lsTable_;\n> +\t\tpayload.lsTableHandleStatic_ = lsTable_.fd();\n> +\t\tipaConfig.payload_.push_back(payload);\n>  \t}\n>  \n>  \tCameraSensorInfo sensorInfo = {};\n> @@ -1157,53 +1167,68 @@ int RPiCameraData::configureIPA()\n>  \t}\n>  \n>  \t/* Ready the IPA - it must know about the sensor resolution. */\n> -\tIPAOperationData result;\n> +\tRPiConfigureParams results;\n>  \n>  \tipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> -\t\t\t&result);\n> +\t\t\t&results);\n>  \n> -\tif (result.operation & 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[0] },\n> -\t\t\t\t\t      { V4L2_CID_EXPOSURE, result.data[1] } });\n> -\t\t\tsensorMetadata_ = result.data[2];\n> +\tfor (RPiConfigurePayload &result : results.payload_) {\n> +\t\tif (result.op_ == RPI_IPA_CONFIG_STAGGERED_WRITE) {\n> +\n> +\t\t\t/*\n> +\t\t\t * Setup our staggered control writer with the sensor default\n> +\t\t\t * gain and exposure delays.\n> +\t\t\t */\n> +\t\t\tif (!staggeredCtrl_) {\n> +\t\t\t\tstaggeredCtrl_.init(unicam_[Unicam::Image].dev(),\n> +\t\t\t\t\t\t{ { V4L2_CID_ANALOGUE_GAIN,\n> +\t\t\t\t\t\t    result.staggeredWriteResult_.gainDelay_ },\n> +\t\t\t\t\t\t  { V4L2_CID_EXPOSURE,\n> +\t\t\t\t\t\t    result.staggeredWriteResult_.exposureDelay_ } });\n> +\t\t\t\tsensorMetadata_ = result.staggeredWriteResult_.sensorMetadata_;\n> +\t\t\t}\n> +\n> +\t\t\t/* Configure the H/V flip controls based on the sensor rotation. */\n> +\t\t\tControlList ctrls(unicam_[Unicam::Image].dev()->controls());\n> +\t\t\tint32_t rotation = sensor_->properties().get(properties::Rotation);\n> +\t\t\tctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation));\n> +\t\t\tctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation));\n> +\t\t\tunicam_[Unicam::Image].dev()->setControls(&ctrls);\n>  \t\t}\n> -\t}\n>  \n> -\tif (result.operation & RPI_IPA_CONFIG_SENSOR) {\n> -\t\tconst ControlList &ctrls = result.controls[0];\n> -\t\tif (!staggeredCtrl_.set(ctrls))\n> -\t\t\tLOG(RPI, Error) << \"V4L2 staggered set failed\";\n> +\t\tif (result.op_ == RPI_IPA_CONFIG_SENSOR) {\n> +\t\t\tconst ControlList &ctrls = result.controls_;\n> +\t\t\tif (!staggeredCtrl_.set(ctrls))\n> +\t\t\t\tLOG(RPI, Error) << \"V4L2 staggered set failed\";\n> +\t\t}\n>  \t}\n>  \n>  \treturn 0;\n>  }\n>  \n>  void RPiCameraData::queueFrameAction([[maybe_unused]] unsigned int frame,\n> -\t\t\t\t     const IPAOperationData &action)\n> +\t\t\t\t     const RPiActionParams &action)\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> +\tswitch (action.op_) {\n>  \tcase RPI_IPA_ACTION_V4L2_SET_STAGGERED: {\n> -\t\tconst ControlList &controls = action.controls[0];\n> +\t\tconst ControlList &controls = action.controls_;\n>  \t\tif (!staggeredCtrl_.set(controls))\n>  \t\t\tLOG(RPI, Error) << \"V4L2 staggered set failed\";\n>  \t\tgoto done;\n>  \t}\n>  \n>  \tcase RPI_IPA_ACTION_V4L2_SET_ISP: {\n> -\t\tControlList controls = action.controls[0];\n> +\t\tControlList controls = action.controls_;\n>  \t\tisp_[Isp::Input].dev()->setControls(&controls);\n>  \t\tgoto done;\n>  \t}\n> +\n> +\tdefault:\n> +\t\tbreak;\n>  \t}\n>  \n>  \tif (state_ == State::Stopped)\n> @@ -1213,20 +1238,20 @@ void RPiCameraData::queueFrameAction([[maybe_unused]] unsigned int frame,\n>  \t * The following actions must not be handled when the pipeline handler\n>  \t * is in a stopped state.\n>  \t */\n> -\tswitch (action.operation) {\n> +\tswitch (action.op_) {\n>  \tcase RPI_IPA_ACTION_STATS_METADATA_COMPLETE: {\n> -\t\tunsigned int bufferId = action.data[0];\n> +\t\tunsigned int bufferId = action.statsComplete_.bufferId_;\n>  \t\tFrameBuffer *buffer = isp_[Isp::Stats].getBuffers()->at(bufferId).get();\n>  \n>  \t\thandleStreamBuffer(buffer, &isp_[Isp::Stats]);\n>  \t\t/* Fill the Request metadata buffer with what the IPA has provided */\n> -\t\trequestQueue_.front()->metadata() = std::move(action.controls[0]);\n> +\t\trequestQueue_.front()->metadata() = std::move(action.statsComplete_.controls_);\n>  \t\tstate_ = State::IpaComplete;\n>  \t\tbreak;\n>  \t}\n>  \n>  \tcase RPI_IPA_ACTION_EMBEDDED_COMPLETE: {\n> -\t\tunsigned int bufferId = action.data[0];\n> +\t\tunsigned int bufferId = action.bufferId_;\n>  \t\tFrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers()->at(bufferId).get();\n>  \t\thandleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n>  \t\tbreak;\n> @@ -1234,20 +1259,17 @@ void RPiCameraData::queueFrameAction([[maybe_unused]] unsigned int frame,\n>  \n>  \tcase RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME:\n>  \tcase RPI_IPA_ACTION_RUN_ISP: {\n> -\t\tunsigned int bufferId = action.data[0];\n> +\t\tunsigned int bufferId = action.bufferId_;\n>  \t\tFrameBuffer *buffer = unicam_[Unicam::Image].getBuffers()->at(bufferId).get();\n>  \n> -\t\tLOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << buffer->cookie()\n> -\t\t\t\t<< \", timestamp: \" << buffer->metadata().timestamp;\n> -\n>  \t\tisp_[Isp::Input].dev()->queueBuffer(buffer);\n> -\t\tdropFrame_ = (action.operation == RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME) ? true : false;\n> +\t\tdropFrame_ = (action.op_ == RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME) ? true : false;\n>  \t\tispOutputCount_ = 0;\n>  \t\tbreak;\n>  \t}\n>  \n>  \tdefault:\n> -\t\tLOG(RPI, Error) << \"Unknown action \" << action.operation;\n> +\t\tLOG(RPI, Error) << \"Unknown action \" << action.op_;\n>  \t\tbreak;\n>  \t}\n>  \n> @@ -1347,10 +1369,10 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n>  \n>  \t/* If this is a stats output, hand it to the IPA now. */\n>  \tif (stream == &isp_[Isp::Stats]) {\n> -\t\tIPAOperationData op;\n> -\t\top.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;\n> -\t\top.data = { RPiIpaMask::STATS | buffer->cookie() };\n> -\t\tipa_->processEvent(op);\n> +\t\tRPiEventParams ev;\n> +\t\tev.ev_ = RPI_IPA_EVENT_SIGNAL_STAT_READY;\n> +\t\tev.bufferId_ = { RPiIpaMask::STATS | buffer->cookie() };\n> +\t\tipa_->processEvent(ev);\n>  \t}\n>  \n>  \thandleState();\n> @@ -1493,7 +1515,7 @@ void RPiCameraData::checkRequestCompleted()\n>  void RPiCameraData::tryRunPipeline()\n>  {\n>  \tFrameBuffer *bayerBuffer, *embeddedBuffer;\n> -\tIPAOperationData op;\n> +\tRPiEventParams ev;\n>  \n>  \t/* If any of our request or buffer queues are empty, we cannot proceed. */\n>  \tif (state_ != State::Idle || requestQueue_.empty() ||\n> @@ -1548,9 +1570,9 @@ 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> +\tev.ev_ = RPI_IPA_EVENT_QUEUE_REQUEST;\n> +\tev.controls_ = { request->controls() };\n> +\tipa_->processEvent(ev);\n>  \n>  \t/* Queue up any ISP buffers passed into the request. */\n>  \tfor (auto &stream : isp_) {\n> @@ -1569,10 +1591,10 @@ void RPiCameraData::tryRunPipeline()\n>  \t\t\t<< \" Bayer buffer id: \" << bayerBuffer->cookie()\n>  \t\t\t<< \" Embedded buffer id: \" << embeddedBuffer->cookie();\n>  \n> -\top.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE;\n> -\top.data = { RPiIpaMask::EMBEDDED_DATA | embeddedBuffer->cookie(),\n> -\t\t    RPiIpaMask::BAYER_DATA | bayerBuffer->cookie() };\n> -\tipa_->processEvent(op);\n> +\tev.ev_ = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE;\n> +\tev.ispPrepare_.embeddedbufferId_ = RPiIpaMask::EMBEDDED_DATA | embeddedBuffer->cookie();\n> +\tev.ispPrepare_.bayerbufferId_    = RPiIpaMask::BAYER_DATA | bayerBuffer->cookie();\n> +\tipa_->processEvent(ev);\n>  }\n>  \n>  void RPiCameraData::tryFlushQueues()\n> -- \n> 2.27.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 5F6E6BF01C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSat, 19 Sep 2020 12:42:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DDF3062FAE;\n\tSat, 19 Sep 2020 14:42:37 +0200 (CEST)","from mail-lf1-x12c.google.com (mail-lf1-x12c.google.com\n\t[IPv6:2a00:1450:4864:20::12c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D0DB560367\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 19 Sep 2020 14:42:36 +0200 (CEST)","by mail-lf1-x12c.google.com with SMTP id w11so9082205lfn.2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 19 Sep 2020 05:42:36 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tz27sm1255819lfg.14.2020.09.19.05.42.35\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 19 Sep 2020 05:42:35 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"R6SMLgxQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=V3J564vD4KepvRDsbLw1fd5iSpNbZNoVofctbkPzLZY=;\n\tb=R6SMLgxQGt3mRNJ1j+w/WLjCNXMz1S7UM0HbeRgOs54q+M7n/sk0KqLKCF/Pe33iMv\n\tWdGXrAmrlhVAZn8gC2AwdYHzdAEiT9lqiZZmxvD1/0Ejkv1TmKZIXfFZfJxLfzvFqPbZ\n\tjT8IPEr80v2tRszA8b6RTlqmbWYCnl2prF+wg84XKFyXr4VmmSzpMPdmXQG+JJXIeuOj\n\ti7G4Mh4LIDm0L0MSMXvvfjKJC/ZWH2EtAk5SU5FYYvg4NSS9D4PA7qd37xBBo5v5Zf38\n\t46mz9CpRcKTag1Qhp1VamH8OCeGORP4NTz5Nhqn3hgtnWU7a3Yhm8gvcPHKjJD4eETDu\n\tZZXQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=V3J564vD4KepvRDsbLw1fd5iSpNbZNoVofctbkPzLZY=;\n\tb=c1tjSWOJ0PXXVKuIxvajIVsaSu/HXOkLsq/b0Etfzk/ZQoPBSrI41k17Q4+o08mPIP\n\tr1gDsMd+s7py5bizRJehC62xsOKvF7PIYoOaX+E2KKwyZ7lOCbi1Kfyw8EAfoQoWniH6\n\t/9YncIyT+RfqeIA2NowuTJ/PdmF3E6ygWaZiAK0glN7y8WfZE9d387cwXzS+6ih8RwsS\n\tto/PmY+0ACf/41LZVSeje9PZp0cNhME6AhOo4AAifCPmWij8UYyH55IJND0zNRM7ZfOa\n\tRumcFdiJ1CrmL+0+T+uqgElWCmCWvP9CuGZWrjsLdWxv2ObZGYIVDpiYDUdOP9jaXABw\n\tAZvQ==","X-Gm-Message-State":"AOAM532n/wHZl5bmH5sjiM2tFiSFKZWjQSMGjAh5vTbuUI//ElX21E7D\n\tqBUaqOAlsthcGnimRNrMoMCLbTE2qJkJlA==","X-Google-Smtp-Source":"ABdhPJySXvxQlyYeYvFfo1oD8sV2hV/PqjUvs5Dbe/JVxCl2zJBEX99TUI66bc0SMMtYcmBdNK3tHQ==","X-Received":"by 2002:a19:bcf:: with SMTP id\n\t198mr11369183lfl.342.1600519355905; \n\tSat, 19 Sep 2020 05:42:35 -0700 (PDT)","Date":"Sat, 19 Sep 2020 14:42:34 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Paul Elder <paul.elder@ideasonboard.com>","Message-ID":"<20200919124234.GT1850958@oden.dyn.berto.se>","References":"<20200915142038.28757-1-paul.elder@ideasonboard.com>\n\t<20200915142038.28757-19-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200915142038.28757-19-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 18/23] 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":12601,"web_url":"https://patchwork.libcamera.org/comment/12601/","msgid":"<20200921041505.GE45948@pyrite.rasen.tech>","date":"2020-09-21T04:15:05","subject":"Re: [libcamera-devel] [PATCH 18/23] 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 Niklas,\n\nThank you for the reivew.\n\nOn Sat, Sep 19, 2020 at 02:42:34PM +0200, Niklas Söderlund wrote:\n> Hi Paul,\n> \n> Thanks for your work.\n> \n> On 2020-09-15 23:20:33 +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 <paul.elder@ideasonboard.com>\n> > ---\n> >  include/libcamera/ipa/raspberrypi.h           |  37 +++---\n> >  src/ipa/raspberrypi/raspberrypi.cpp           | 108 +++++++++-------\n> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 120 +++++++++++-------\n> >  3 files changed, 155 insertions(+), 110 deletions(-)\n> > \n> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > index ed2b12d5..e58d1cbc 100644\n> > --- a/include/libcamera/ipa/raspberrypi.h\n> > +++ b/include/libcamera/ipa/raspberrypi.h\n> > @@ -23,22 +23,27 @@ enum RPiIpaMask {\n> >  namespace libcamera {\n> >  \n> >  /* List of controls handled by the Raspberry Pi IPA */\n> > -static const ControlInfoMap RPiControls = {\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(0, static_cast<int32_t>(controls::MeteringModeMax)) },\n> > -\t{ &controls::AeConstraintMode, ControlInfo(0, static_cast<int32_t>(controls::ConstraintModeMax)) },\n> > -\t{ &controls::AeExposureMode, ControlInfo(0, static_cast<int32_t>(controls::ExposureModeMax)) },\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(0, static_cast<int32_t>(controls::AwbModeMax)) },\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> > +static ControlInfoMap RPiControls;\n> > +\n> > +inline void initializeRPiControls()\n> > +{\n> > +\tRPiControls = {\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(0, static_cast<int32_t>(controls::MeteringModeMax)) },\n> > +\t\t{ &controls::AeConstraintMode, ControlInfo(0, static_cast<int32_t>(controls::ConstraintModeMax)) },\n> > +\t\t{ &controls::AeExposureMode, ControlInfo(0, static_cast<int32_t>(controls::ExposureModeMax)) },\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(0, static_cast<int32_t>(controls::AwbModeMax)) },\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> > +\t};\n> \n> Why do you nee to move this to a function?\n\nOtherwise RPiControls gets constructed too early, and if an error\nhappens in ControlInfoMap::generateIdmap() and it tries to LOG, it'll\ncause a segfault. So it's either construct RPiControls later, or remove\nthe error LOG (or add protection to LOG?).\n\n> >  };\n> >  \n> >  } /* namespace libcamera */\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 4557016c..e09caa8d 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -19,6 +19,7 @@\n> >  #include <libcamera/ipa/ipa_interface.h>\n> >  #include <libcamera/ipa/ipa_module_info.h>\n> >  #include <libcamera/ipa/raspberrypi.h>\n> > +#include <libcamera/ipa/raspberrypi_generated.h>\n> >  #include <libcamera/request.h>\n> >  #include <libcamera/span.h>\n> >  \n> > @@ -60,7 +61,7 @@ namespace libcamera {\n> >  \n> >  LOG_DEFINE_CATEGORY(IPARPI)\n> >  \n> > -class IPARPi : public IPAInterface\n> > +class IPARPi : public IPARPiInterface\n> >  {\n> >  public:\n> >  \tIPARPi()\n> > @@ -68,6 +69,7 @@ public:\n> >  \t\t  frame_count_(0), check_count_(0), hide_count_(0),\n> >  \t\t  mistrust_count_(0), lsTable_(nullptr)\n> >  \t{\n> > +\t\tinitializeRPiControls();\n> >  \t}\n> >  \n> >  \t~IPARPi()\n> > @@ -82,12 +84,12 @@ public:\n> >  \n> >  \tvoid configure(const CameraSensorInfo &sensorInfo,\n> >  \t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n> > -\t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls,\n> > -\t\t       const IPAOperationData &data,\n> > -\t\t       IPAOperationData *response) override;\n> > +\t\t       const std::map<unsigned int, ControlInfoMap> &entityControls,\n> \n> Is it by design or a effect/limitation of mojo we can't keep const & ?\n\nYes, mojo (the language) doesn't support const nor & in array\ndefinitions.\n\n> > +\t\t       const RPiConfigureParams &data,\n> > +\t\t       RPiConfigureParams *response) override;\n> >  \tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override;\n> >  \tvoid unmapBuffers(const std::vector<unsigned int> &ids) override;\n> > -\tvoid processEvent(const IPAOperationData &event) override;\n> > +\tvoid processEvent(const RPiEventParams &event) override;\n> >  \n> >  private:\n> >  \tvoid setMode(const CameraSensorInfo &sensorInfo);\n> > @@ -143,6 +145,11 @@ private:\n> >  \tunsigned int mistrust_count_;\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> > @@ -192,15 +199,13 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)\n> >  \n> >  void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> >  \t\t       [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,\n> > -\t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls,\n> > -\t\t       const IPAOperationData &ipaConfig,\n> > -\t\t       IPAOperationData *result)\n> > +\t\t       const std::map<unsigned int, ControlInfoMap> &entityControls,\n> > +\t\t       const RPiConfigureParams &ipaConfig,\n> > +\t\t       RPiConfigureParams *result)\n> >  {\n> >  \tif (entityControls.empty())\n> >  \t\treturn;\n> >  \n> > -\tresult->operation = 0;\n> > -\n> >  \tunicam_ctrls_ = entityControls.at(0);\n> >  \tisp_ctrls_ = entityControls.at(1);\n> >  \t/* Setup a metadata ControlList to output metadata. */\n> > @@ -222,11 +227,13 @@ 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> > +\t\tRPiConfigurePayload payload = {};\n> > +\t\tpayload.op_ = RPI_IPA_CONFIG_STAGGERED_WRITE;\n> > +\t\tpayload.staggeredWriteResult_.gainDelay_ = gainDelay;\n> > +\t\tpayload.staggeredWriteResult_.exposureDelay_ = exposureDelay;\n> > +\t\tpayload.staggeredWriteResult_.sensorMetadata_ = sensorMetadata;\n> \n> So much nicer, I really like this!\n\n\\o/\n\n> >  \n> > -\t\tresult->operation |= RPI_IPA_CONFIG_STAGGERED_WRITE;\n> > +\t\tresult->payload_.push_back(payload);\n> >  \t}\n> >  \n> >  \t/* Re-assemble camera mode using the sensor info. */\n> > @@ -274,15 +281,23 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> >  \tif (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {\n> >  \t\tControlList ctrls(unicam_ctrls_);\n> >  \t\tapplyAGC(&agcStatus, ctrls);\n> > -\t\tresult->controls.push_back(ctrls);\n> >  \n> > -\t\tresult->operation |= RPI_IPA_CONFIG_SENSOR;\n> > +\t\tRPiConfigurePayload payload = {};\n> > +\t\tpayload.op_ = RPI_IPA_CONFIG_SENSOR;\n> > +\t\tpayload.controls_ = ctrls;\n> > +\n> > +\t\tresult->payload_.push_back(payload);\n> >  \t}\n> >  \n> >  \tlastMode_ = mode_;\n> >  \n> >  \t/* Store the lens shading table pointer and handle if available. */\n> > -\tif (ipaConfig.operation & RPI_IPA_CONFIG_LS_TABLE) {\n> > +\tauto lens = std::find_if(ipaConfig.payload_.begin(),\n> > +\t\t\t\t ipaConfig.payload_.end(),\n> > +\t\t\t\t [] (const RPiConfigurePayload &p) {\n> > +\t\t\t\t\t return p.op_ == RPI_IPA_CONFIG_LS_TABLE;\n> > +\t\t\t\t });\n> \n> I think this warrents a helper, is it possible to have a base class for \n> RPiConfigureParams which could be shared between pipelines to have such \n> helpers or do we think that is to restricitve and should do so for each \n> IPA protocol implementation?\n\nI don't think that's the right solution. I think the proper way to make\nthis nicer is with a better design of the RPi IPA interface.\nCONFIG_LS_TABLE is only used pipeline -> IPA and CONFIG_STAGGERED_WRITE\nand CONFIG_SENSOR are IPA -> pipeline. Perhaps a separate configure()\ninput and output parameter type? Like:\n\nstruct RPiConfigInput {\n\tbool lsTableIsValid = false;\n\tFileDescriptor lsTableHandle;\n\tint32 lsTableHandleStatic = -1;\n};\n\nstruct RPiConfigOutput {\n\tbool staggeredWriteIsValid = false;\n\tRPiStaggeredWritePayload staggeredWriteResult;\n\tbool configSensor = false;\n\tControlList controls;\n};\n\nI discussed with Laurent about using nullable fields in structs (like\nmojo supports) but that had more demerits than embedding did imo so\nflags it is.\n\nThen this and the next hunk would become:\n\n@@ -274,15 +281,23 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n \tif (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {\n \t\tControlList ctrls(unicam_ctrls_);\n \t\tapplyAGC(&agcStatus, ctrls);\n-\t\tresult->controls.push_back(ctrls);\n \n-\t\tresult->operation |= RPI_IPA_CONFIG_SENSOR;\n+\t\tresult->configSensor_ = true;\n+\t\tresult->controls_ = ctrls;\n \t}\n \n \tlastMode_ = mode_;\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.lsTableIsValid_) {\n \t\t/* Remove any previous table, if there was one. */\n \t\tif (lsTable_) {\n \t\t\tmunmap(lsTable_, MAX_LS_GRID_SIZE);\n@@ -290,7 +305,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n \t\t}\n \n \t\t/* Map the LS table buffer into user space. */\n-\t\tlsTableHandle_ = FileDescriptor(ipaConfig.data[0]);\n+\t\tlsTableHandle_ = FileDescriptor(ipaConfig.lsTableHandle_);\n+\t\tlsTableHandlePH_ = ipaConfig.lsTableHandleStatic_;\n \t\tif (lsTableHandle_.isValid()) {\n \t\t\tlsTable_ = mmap(nullptr, MAX_LS_GRID_SIZE, PROT_READ | PROT_WRITE,\n \t\t\t\t\tMAP_SHARED, lsTableHandle_.fd(), 0);\n\nOr something along those lines. I had just directly copied the old IPA\ninterface with IPAOperationData, so even I haven't yet used the custom\nIPA interface to its fullest potential.\n\n\nPaul\n\n> > +\tif (lens != ipaConfig.payload_.end()) {\n> >  \t\t/* Remove any previous table, if there was one. */\n> >  \t\tif (lsTable_) {\n> >  \t\t\tmunmap(lsTable_, MAX_LS_GRID_SIZE);\n> > @@ -290,7 +305,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> >  \t\t}\n> >  \n> >  \t\t/* Map the LS table buffer into user space. */\n> > -\t\tlsTableHandle_ = FileDescriptor(ipaConfig.data[0]);\n> > +\t\tlsTableHandle_ = FileDescriptor(lens->lsTableHandle_);\n> > +\t\tlsTableHandlePH_ = lens->lsTableHandleStatic_;\n> >  \t\tif (lsTableHandle_.isValid()) {\n> >  \t\t\tlsTable_ = mmap(nullptr, MAX_LS_GRID_SIZE, PROT_READ | PROT_WRITE,\n> >  \t\t\t\t\tMAP_SHARED, lsTableHandle_.fd(), 0);\n> > @@ -335,11 +351,11 @@ void IPARPi::unmapBuffers(const std::vector<unsigned int> &ids)\n> >  \t}\n> >  }\n> >  \n> > -void IPARPi::processEvent(const IPAOperationData &event)\n> > +void IPARPi::processEvent(const RPiEventParams &event)\n> >  {\n> > -\tswitch (event.operation) {\n> > +\tswitch (event.ev_) {\n> >  \tcase RPI_IPA_EVENT_SIGNAL_STAT_READY: {\n> > -\t\tunsigned int bufferId = event.data[0];\n> > +\t\tunsigned int bufferId = event.bufferId_;\n> >  \n> >  \t\tif (++check_count_ != frame_count_) /* assert here? */\n> >  \t\t\tLOG(IPARPI, Error) << \"WARNING: Prepare/Process mismatch!!!\";\n> > @@ -348,17 +364,17 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> >  \n> >  \t\treportMetadata();\n> >  \n> > -\t\tIPAOperationData op;\n> > -\t\top.operation = RPI_IPA_ACTION_STATS_METADATA_COMPLETE;\n> > -\t\top.data = { bufferId & RPiIpaMask::ID };\n> > -\t\top.controls = { libcameraMetadata_ };\n> > +\t\tRPiActionParams op;\n> > +\t\top.op_ = RPI_IPA_ACTION_STATS_METADATA_COMPLETE;\n> > +\t\top.statsComplete_.bufferId_ = { bufferId & RPiIpaMask::ID };\n> > +\t\top.statsComplete_.controls_ = { libcameraMetadata_ };\n> >  \t\tqueueFrameAction.emit(0, op);\n> >  \t\tbreak;\n> >  \t}\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> > +\t\tunsigned int embeddedbufferId = event.ispPrepare_.embeddedbufferId_;\n> > +\t\tunsigned int bayerbufferId = event.ispPrepare_.bayerbufferId_;\n> >  \n> >  \t\t/*\n> >  \t\t * At start-up, or after a mode-switch, we may want to\n> > @@ -368,23 +384,23 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> >  \t\tprepareISP(embeddedbufferId);\n> >  \n> >  \t\t/* Ready to push the input buffer into the ISP. */\n> > -\t\tIPAOperationData op;\n> > +\t\tRPiActionParams op;\n> >  \t\tif (++frame_count_ > hide_count_)\n> > -\t\t\top.operation = RPI_IPA_ACTION_RUN_ISP;\n> > +\t\t\top.op_ = RPI_IPA_ACTION_RUN_ISP;\n> >  \t\telse\n> > -\t\t\top.operation = RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME;\n> > -\t\top.data = { bayerbufferId & RPiIpaMask::ID };\n> > +\t\t\top.op_ = RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME;\n> > +\t\top.bufferId_ = { bayerbufferId & RPiIpaMask::ID };\n> >  \t\tqueueFrameAction.emit(0, op);\n> >  \t\tbreak;\n> >  \t}\n> >  \n> >  \tcase RPI_IPA_EVENT_QUEUE_REQUEST: {\n> > -\t\tqueueRequest(event.controls[0]);\n> > +\t\tqueueRequest(event.controls_);\n> >  \t\tbreak;\n> >  \t}\n> >  \n> >  \tdefault:\n> > -\t\tLOG(IPARPI, Error) << \"Unknown event \" << event.operation;\n> > +\t\tLOG(IPARPI, Error) << \"Unknown event \" << event.ev_;\n> >  \t\tbreak;\n> >  \t}\n> >  }\n> > @@ -489,6 +505,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> >  \tfor (auto const &ctrl : controls) {\n> >  \t\tLOG(IPARPI, Info) << \"Request ctrl: \"\n> >  \t\t\t\t  << controls::controls.at(ctrl.first)->name()\n> > @@ -698,9 +716,9 @@ 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 & RPiIpaMask::ID };\n> > +\tRPiActionParams op;\n> > +\top.op_ = RPI_IPA_ACTION_EMBEDDED_COMPLETE;\n> > +\top.bufferId_ = { bufferId & RPiIpaMask::ID };\n> >  \tqueueFrameAction.emit(0, op);\n> >  }\n> >  \n> > @@ -763,9 +781,9 @@ void IPARPi::prepareISP(unsigned int bufferId)\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\tRPiActionParams op;\n> > +\t\t\top.op_ = RPI_IPA_ACTION_V4L2_SET_ISP;\n> > +\t\t\top.controls_ = ctrls;\n> >  \t\t\tqueueFrameAction.emit(0, op);\n> >  \t\t}\n> >  \t}\n> > @@ -823,9 +841,9 @@ void IPARPi::processStats(unsigned int bufferId)\n> >  \t\tControlList ctrls(unicam_ctrls_);\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\tRPiActionParams op;\n> > +\t\top.op_ = RPI_IPA_ACTION_V4L2_SET_STAGGERED;\n> > +\t\top.controls_ = ctrls;\n> >  \t\tqueueFrameAction.emit(0, op);\n> >  \t}\n> >  }\n> > @@ -1056,7 +1074,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> >  \t\t.ref_transform = 0,\n> >  \t\t.corner_sampled = 1,\n> >  \t\t.gain_format = GAIN_FORMAT_U4P10\n> > @@ -1136,9 +1154,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<IPARPi>());\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 ce43af34..70bb6fcc 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -15,7 +15,9 @@\n> >  #include <libcamera/control_ids.h>\n> >  #include <libcamera/file_descriptor.h>\n> >  #include <libcamera/formats.h>\n> > +#include <libcamera/ipa/ipa_proxy_raspberrypi.h>\n> >  #include <libcamera/ipa/raspberrypi.h>\n> > +#include <libcamera/ipa/raspberrypi_generated.h>\n> >  #include <libcamera/logging.h>\n> >  #include <libcamera/property_ids.h>\n> >  #include <libcamera/request.h>\n> > @@ -296,7 +298,7 @@ public:\n> >  \tint loadIPA();\n> >  \tint configureIPA();\n> >  \n> > -\tvoid queueFrameAction(unsigned int frame, const IPAOperationData &action);\n> > +\tvoid queueFrameAction(unsigned int frame, const RPiActionParams &action);\n> >  \n> >  \t/* bufferComplete signal handlers. */\n> >  \tvoid unicamBufferDequeue(FrameBuffer *buffer);\n> > @@ -307,6 +309,8 @@ public:\n> >  \tvoid handleStreamBuffer(FrameBuffer *buffer, const RPiStream *stream);\n> >  \tvoid handleState();\n> >  \n> > +\tstd::unique_ptr<IPAProxyRPi> ipa_;\n> > +\n> >  \tCameraSensor *sensor_;\n> >  \t/* Array of Unicam and ISP device streams and associated buffers/streams. */\n> >  \tRPiDevice<Unicam, 2> unicam_;\n> > @@ -506,6 +510,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> >  PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)\n> >  \t: PipelineHandler(manager), unicam_(nullptr), isp_(nullptr)\n> >  {\n> > +\tinitializeRPiControls();\n> >  }\n> >  \n> >  CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > @@ -1103,7 +1108,9 @@ void RPiCameraData::frameStarted(uint32_t sequence)\n> >  \n> >  int RPiCameraData::loadIPA()\n> >  {\n> > -\tipa_ = IPAManager::createIPA(pipe_, 1, 1);\n> > +\tstd::unique_ptr<IPAProxy> ptr = IPAManager::createIPA(pipe_, 1, 1);\n> > +\tipa_ = std::unique_ptr<IPAProxyRPi>{static_cast<IPAProxyRPi*>(std::move(ptr).release())};\n> > +\n> >  \tif (!ipa_)\n> >  \t\treturn -ENOENT;\n> >  \n> > @@ -1119,8 +1126,8 @@ int RPiCameraData::loadIPA()\n> >  int RPiCameraData::configureIPA()\n> >  {\n> >  \tstd::map<unsigned int, IPAStream> streamConfig;\n> > -\tstd::map<unsigned int, const ControlInfoMap &> entityControls;\n> > -\tIPAOperationData ipaConfig = {};\n> > +\tstd::map<unsigned int, ControlInfoMap> entityControls;\n> > +\tRPiConfigureParams ipaConfig;\n> >  \n> >  \t/* Get the device format to pass to the IPA. */\n> >  \tV4L2DeviceFormat sensorFormat;\n> > @@ -1145,8 +1152,11 @@ int RPiCameraData::configureIPA()\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 = { static_cast<unsigned int>(lsTable_.fd()) };\n> > +\t\tRPiConfigurePayload payload;\n> > +\t\tpayload.op_ = RPI_IPA_CONFIG_LS_TABLE;\n> > +\t\tpayload.lsTableHandle_ = lsTable_;\n> > +\t\tpayload.lsTableHandleStatic_ = lsTable_.fd();\n> > +\t\tipaConfig.payload_.push_back(payload);\n> >  \t}\n> >  \n> >  \tCameraSensorInfo sensorInfo = {};\n> > @@ -1157,53 +1167,68 @@ int RPiCameraData::configureIPA()\n> >  \t}\n> >  \n> >  \t/* Ready the IPA - it must know about the sensor resolution. */\n> > -\tIPAOperationData result;\n> > +\tRPiConfigureParams results;\n> >  \n> >  \tipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> > -\t\t\t&result);\n> > +\t\t\t&results);\n> >  \n> > -\tif (result.operation & 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[0] },\n> > -\t\t\t\t\t      { V4L2_CID_EXPOSURE, result.data[1] } });\n> > -\t\t\tsensorMetadata_ = result.data[2];\n> > +\tfor (RPiConfigurePayload &result : results.payload_) {\n> > +\t\tif (result.op_ == RPI_IPA_CONFIG_STAGGERED_WRITE) {\n> > +\n> > +\t\t\t/*\n> > +\t\t\t * Setup our staggered control writer with the sensor default\n> > +\t\t\t * gain and exposure delays.\n> > +\t\t\t */\n> > +\t\t\tif (!staggeredCtrl_) {\n> > +\t\t\t\tstaggeredCtrl_.init(unicam_[Unicam::Image].dev(),\n> > +\t\t\t\t\t\t{ { V4L2_CID_ANALOGUE_GAIN,\n> > +\t\t\t\t\t\t    result.staggeredWriteResult_.gainDelay_ },\n> > +\t\t\t\t\t\t  { V4L2_CID_EXPOSURE,\n> > +\t\t\t\t\t\t    result.staggeredWriteResult_.exposureDelay_ } });\n> > +\t\t\t\tsensorMetadata_ = result.staggeredWriteResult_.sensorMetadata_;\n> > +\t\t\t}\n> > +\n> > +\t\t\t/* Configure the H/V flip controls based on the sensor rotation. */\n> > +\t\t\tControlList ctrls(unicam_[Unicam::Image].dev()->controls());\n> > +\t\t\tint32_t rotation = sensor_->properties().get(properties::Rotation);\n> > +\t\t\tctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation));\n> > +\t\t\tctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation));\n> > +\t\t\tunicam_[Unicam::Image].dev()->setControls(&ctrls);\n> >  \t\t}\n> > -\t}\n> >  \n> > -\tif (result.operation & RPI_IPA_CONFIG_SENSOR) {\n> > -\t\tconst ControlList &ctrls = result.controls[0];\n> > -\t\tif (!staggeredCtrl_.set(ctrls))\n> > -\t\t\tLOG(RPI, Error) << \"V4L2 staggered set failed\";\n> > +\t\tif (result.op_ == RPI_IPA_CONFIG_SENSOR) {\n> > +\t\t\tconst ControlList &ctrls = result.controls_;\n> > +\t\t\tif (!staggeredCtrl_.set(ctrls))\n> > +\t\t\t\tLOG(RPI, Error) << \"V4L2 staggered set failed\";\n> > +\t\t}\n> >  \t}\n> >  \n> >  \treturn 0;\n> >  }\n> >  \n> >  void RPiCameraData::queueFrameAction([[maybe_unused]] unsigned int frame,\n> > -\t\t\t\t     const IPAOperationData &action)\n> > +\t\t\t\t     const RPiActionParams &action)\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> > +\tswitch (action.op_) {\n> >  \tcase RPI_IPA_ACTION_V4L2_SET_STAGGERED: {\n> > -\t\tconst ControlList &controls = action.controls[0];\n> > +\t\tconst ControlList &controls = action.controls_;\n> >  \t\tif (!staggeredCtrl_.set(controls))\n> >  \t\t\tLOG(RPI, Error) << \"V4L2 staggered set failed\";\n> >  \t\tgoto done;\n> >  \t}\n> >  \n> >  \tcase RPI_IPA_ACTION_V4L2_SET_ISP: {\n> > -\t\tControlList controls = action.controls[0];\n> > +\t\tControlList controls = action.controls_;\n> >  \t\tisp_[Isp::Input].dev()->setControls(&controls);\n> >  \t\tgoto done;\n> >  \t}\n> > +\n> > +\tdefault:\n> > +\t\tbreak;\n> >  \t}\n> >  \n> >  \tif (state_ == State::Stopped)\n> > @@ -1213,20 +1238,20 @@ void RPiCameraData::queueFrameAction([[maybe_unused]] unsigned int frame,\n> >  \t * The following actions must not be handled when the pipeline handler\n> >  \t * is in a stopped state.\n> >  \t */\n> > -\tswitch (action.operation) {\n> > +\tswitch (action.op_) {\n> >  \tcase RPI_IPA_ACTION_STATS_METADATA_COMPLETE: {\n> > -\t\tunsigned int bufferId = action.data[0];\n> > +\t\tunsigned int bufferId = action.statsComplete_.bufferId_;\n> >  \t\tFrameBuffer *buffer = isp_[Isp::Stats].getBuffers()->at(bufferId).get();\n> >  \n> >  \t\thandleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> >  \t\t/* Fill the Request metadata buffer with what the IPA has provided */\n> > -\t\trequestQueue_.front()->metadata() = std::move(action.controls[0]);\n> > +\t\trequestQueue_.front()->metadata() = std::move(action.statsComplete_.controls_);\n> >  \t\tstate_ = State::IpaComplete;\n> >  \t\tbreak;\n> >  \t}\n> >  \n> >  \tcase RPI_IPA_ACTION_EMBEDDED_COMPLETE: {\n> > -\t\tunsigned int bufferId = action.data[0];\n> > +\t\tunsigned int bufferId = action.bufferId_;\n> >  \t\tFrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers()->at(bufferId).get();\n> >  \t\thandleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> >  \t\tbreak;\n> > @@ -1234,20 +1259,17 @@ void RPiCameraData::queueFrameAction([[maybe_unused]] unsigned int frame,\n> >  \n> >  \tcase RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME:\n> >  \tcase RPI_IPA_ACTION_RUN_ISP: {\n> > -\t\tunsigned int bufferId = action.data[0];\n> > +\t\tunsigned int bufferId = action.bufferId_;\n> >  \t\tFrameBuffer *buffer = unicam_[Unicam::Image].getBuffers()->at(bufferId).get();\n> >  \n> > -\t\tLOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << buffer->cookie()\n> > -\t\t\t\t<< \", timestamp: \" << buffer->metadata().timestamp;\n> > -\n> >  \t\tisp_[Isp::Input].dev()->queueBuffer(buffer);\n> > -\t\tdropFrame_ = (action.operation == RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME) ? true : false;\n> > +\t\tdropFrame_ = (action.op_ == RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME) ? true : false;\n> >  \t\tispOutputCount_ = 0;\n> >  \t\tbreak;\n> >  \t}\n> >  \n> >  \tdefault:\n> > -\t\tLOG(RPI, Error) << \"Unknown action \" << action.operation;\n> > +\t\tLOG(RPI, Error) << \"Unknown action \" << action.op_;\n> >  \t\tbreak;\n> >  \t}\n> >  \n> > @@ -1347,10 +1369,10 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n> >  \n> >  \t/* If this is a stats output, hand it to the IPA now. */\n> >  \tif (stream == &isp_[Isp::Stats]) {\n> > -\t\tIPAOperationData op;\n> > -\t\top.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;\n> > -\t\top.data = { RPiIpaMask::STATS | buffer->cookie() };\n> > -\t\tipa_->processEvent(op);\n> > +\t\tRPiEventParams ev;\n> > +\t\tev.ev_ = RPI_IPA_EVENT_SIGNAL_STAT_READY;\n> > +\t\tev.bufferId_ = { RPiIpaMask::STATS | buffer->cookie() };\n> > +\t\tipa_->processEvent(ev);\n> >  \t}\n> >  \n> >  \thandleState();\n> > @@ -1493,7 +1515,7 @@ void RPiCameraData::checkRequestCompleted()\n> >  void RPiCameraData::tryRunPipeline()\n> >  {\n> >  \tFrameBuffer *bayerBuffer, *embeddedBuffer;\n> > -\tIPAOperationData op;\n> > +\tRPiEventParams ev;\n> >  \n> >  \t/* If any of our request or buffer queues are empty, we cannot proceed. */\n> >  \tif (state_ != State::Idle || requestQueue_.empty() ||\n> > @@ -1548,9 +1570,9 @@ 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> > +\tev.ev_ = RPI_IPA_EVENT_QUEUE_REQUEST;\n> > +\tev.controls_ = { request->controls() };\n> > +\tipa_->processEvent(ev);\n> >  \n> >  \t/* Queue up any ISP buffers passed into the request. */\n> >  \tfor (auto &stream : isp_) {\n> > @@ -1569,10 +1591,10 @@ void RPiCameraData::tryRunPipeline()\n> >  \t\t\t<< \" Bayer buffer id: \" << bayerBuffer->cookie()\n> >  \t\t\t<< \" Embedded buffer id: \" << embeddedBuffer->cookie();\n> >  \n> > -\top.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE;\n> > -\top.data = { RPiIpaMask::EMBEDDED_DATA | embeddedBuffer->cookie(),\n> > -\t\t    RPiIpaMask::BAYER_DATA | bayerBuffer->cookie() };\n> > -\tipa_->processEvent(op);\n> > +\tev.ev_ = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE;\n> > +\tev.ispPrepare_.embeddedbufferId_ = RPiIpaMask::EMBEDDED_DATA | embeddedBuffer->cookie();\n> > +\tev.ispPrepare_.bayerbufferId_    = RPiIpaMask::BAYER_DATA | bayerBuffer->cookie();\n> > +\tipa_->processEvent(ev);\n> >  }\n> >  \n> >  void RPiCameraData::tryFlushQueues()\n> > -- \n> > 2.27.0\n> > \n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n> \n> -- \n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 86BB0C3B5B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 21 Sep 2020 04:15:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D72BF62FD2;\n\tMon, 21 Sep 2020 06:15:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AEEBF60363\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 21 Sep 2020 06:15:16 +0200 (CEST)","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 CA4AE27B;\n\tMon, 21 Sep 2020 06:15:11 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"jr5nya94\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600661716;\n\tbh=v31EVVbmYve+F5bsJFmTO+AECLmpNBuMBxcKQBJFVgY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jr5nya944sRh+7Z0U+5OjKYqxhME889YHX+d62N//wJTiH7/Euejoy/nWzNShowef\n\th52ggLw5ru+i760Py1UR7tGovuR29Ijmb3UfzaUMLClPV8nLxZ9VI9/aBUTXk1RIAk\n\tId4CTkS5v5lGRGXIFWgOisI97GhyUlJA5ILRPANg=","Date":"Mon, 21 Sep 2020 13:15:05 +0900","From":"paul.elder@ideasonboard.com","To":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","Message-ID":"<20200921041505.GE45948@pyrite.rasen.tech>","References":"<20200915142038.28757-1-paul.elder@ideasonboard.com>\n\t<20200915142038.28757-19-paul.elder@ideasonboard.com>\n\t<20200919124234.GT1850958@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200919124234.GT1850958@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH 18/23] 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]