| Message ID | 20210901143800.119118-1-jacopo@jmondi.org |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
On 01/09/2021 15:37, Jacopo Mondi wrote: > The IPA::configure() function has an IPAConfigInfo parameters which > contains a map of numerical indexes to ControlInfoMap instances. > > This is a leftover of the old IPA protocol, where it was not possible to > specify a rich interface as it is possible today and each entity > ControlInfoMap was indexed by a numerical id and stored in a map. > > Now that the IPA interface allows to specify parameters by name, drop the > map and send the sensor's control info map only. > > If we'll need more ControlInfoMap to be shared with the IPA, a new parameter > can be added to IPAConfigInfo. It looks like this will need a patch for https://git.libcamera.org/libcamera/ipu3-ipa.git/ as well. But given that it requires manually installed AIQ libraries on non CrOS build environments, perhaps it's something myself or Umang should handle when this series merges. I wonder if we should be increasing the IPA ABI / version numbers too whenever we make a change. But alas - it doesn't look like that's fully implemented, The mojo files should have a version number in there (or even better, could mojo create a hash of the interface to catch when it changes I wonder)? So, given that you can't update the other IPA, and you can't update the IPU3 IPA Version (without further patches) ... I guess this is it ... Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/ipa/ipu3.mojom | 2 +- > src/ipa/ipu3/ipu3.cpp | 4 ++-- > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- > 3 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom > index d561c2244907..2045ce909a88 100644 > --- a/include/libcamera/ipa/ipu3.mojom > +++ b/include/libcamera/ipa/ipu3.mojom > @@ -32,7 +32,7 @@ struct IPU3Action { > > struct IPAConfigInfo { > libcamera.IPACameraSensorInfo sensorInfo; > - map<uint32, libcamera.ControlInfoMap> entityControls; > + libcamera.ControlInfoMap sensorControls; > libcamera.Size bdsOutputSize; > libcamera.Size iif; > }; > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index c925cf642611..6a74f7826332 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -337,14 +337,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize) > > int IPAIPU3::configure(const IPAConfigInfo &configInfo) > { > - if (configInfo.entityControls.empty()) { > + if (configInfo.sensorControls.empty()) { > LOG(IPAIPU3, Error) << "No controls provided"; > return -ENODATA; > } > > sensorInfo_ = configInfo.sensorInfo; > > - ctrls_ = configInfo.entityControls.at(0); > + ctrls_ = configInfo.sensorControls; > > const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE); > if (itExp == ctrls_.end()) { > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index c287bf86e79a..92e869257e53 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -654,7 +654,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > } > > ipa::ipu3::IPAConfigInfo configInfo; > - configInfo.entityControls.emplace(0, data->cio2_.sensor()->controls()); > + configInfo.sensorControls = data->cio2_.sensor()->controls(); > configInfo.sensorInfo = sensorInfo; > configInfo.bdsOutputSize = config->imguConfig().bds; > configInfo.iif = config->imguConfig().iif; >
On 01/09/2021 15:37, Jacopo Mondi wrote: > When running the IPA in isolated mode, each side of the IPC boundary > has an instance of the ControlSerializer class which is used to > serializer/deserialize controls before transmitting them on the wire. > > The IPAProxyWorker, which creates and manages the process the IPA runs into, > does not reset its ControlSerializer upon an IPA::configure() call, while > the IPAProxy does so, effectively creating a misalignment between the > two sides of the fence. > > This obviously creates issues as one side of the IPC runs with a > populated and possibly stale cache of ControlInfoMap references, while the > other side gets reset every time a new configuration is applied to the > Camera. > > Fix that by resetting the IPAProxyWorker ControlSerializer on an > IPA configure() call. > > This change fixes an issue which is easily triggered by running two > consecutive capture sessions with the IPA running in isolated mode: > ERROR Serializer control_serializer.cpp:520 Can't deserialize ControlList: unknown ControlInfoMap > > Fixes: 7832e19a599e ("utils: ipc: add templates for code generation for IPC mechanism") Is it also technically fixing the patch that added the reset on the other side? as that's when the misalignment begins? > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl > index 8a88bd467da7..22cbc15c5eff 100644 > --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl > @@ -79,6 +79,10 @@ public: > > {% for method in interface_main.methods %} > case {{cmd_enum_name}}::{{method.mojom_name|cap}}: { > + > +{%- if method.mojom_name == "configure" %} > + controlSerializer_.reset(); one more level indentation >> ? It looks like we're "inside" this case statement, so it should be one level deeper. > +{%- endif %} Wow, these templates are hard to parse :-) But ... it looks like this is right ;-) Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > {{proxy_funcs.deserialize_call(method|method_param_inputs, '_ipcMessage.data()', '_ipcMessage.fds()', false, true)|indent(8, true)}} > {% for param in method|method_param_outputs %} > {{param|name}} {{param.mojom_name}}; >
On 01/09/2021 15:37, Jacopo Mondi wrote: > When running the IPA in isolated mode, each side of the IPC boundary > has an instance of the ControlSerializer class. > > Each serializer instance tags with a numerical id (handle) each > ControlInfoMap instance it serializes, to be capable of associating > ControlList with it at serialization and deserialization time, and > increments the numerical counter for each newly serialized control info > map. > > Having two instances of the ControlSerializer class running in two > different process spaces, each instance increments its own counter, > preventing from maintaining a globally shared state where each > ControlInfoMap instance is reference by a unique identifier. > > Fix this by advancing the serial_ counter at ControlInfoMap > de-serialization time, so that each newly serialized map will have a > globally unique identifier. > > Fixes: 2c5f0ad23aa4 ("libcamera: Add controls serializer") > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/control_serializer.cpp | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > index 27360526f9eb..08cfecca3078 100644 > --- a/src/libcamera/control_serializer.cpp > +++ b/src/libcamera/control_serializer.cpp > @@ -380,6 +380,9 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > return {}; > } > > + /* Keep the info map handles in sync between the two sides of IPC. */ > + serial_ = std::max(serial_, hdr->handle); > + Ok, I can see how this works now. Every serialize gets deserialized, and at that point you know both sides have the same serial_. So whichever side next calls serialize() will be the one that increments, and the other side will then update accordingly when it has it's deser... Took me a longer time to realise than I liked - but it's actually simple enough ;-) Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > /* > * Use the ControlIdMap corresponding to the id map type. If the type > * references a globally defined id map (such as controls::controls >
On 01/09/2021 15:37, Jacopo Mondi wrote: > When a ControlList is deserialized, the code searches for a valid > ControlInfoMap in the local cache and use its id map to initialize the > list. If no valid ControlInfoMap is found, as it's usually the case > for lists transporting libcamera controls and properties, the globally > defined controls::controls id map is used unconditionally. > > This breaks the deserialization of libcamera properties, for which a > wrong idmap is used at construction time. > > As the serialization header now transports an id_map_type field, store > the idmap type at serialization time, and re-use it at > deserialization time to identify the correct id map. > > Also make the validation stricter by imposing to list of V4L2 controls to > have an associated ControlInfoMap available, as there is no globally > defined idmap for such controls. > > To be able to retrieve the idmap associated with a ControlList, add an > accessor function to the ControlList class. > > It might be worth in future using a ControlInfoMap to initialize the > deserialized ControlList to implement controls validation against their > limit. As such validation is not implemented at the moment, maintain the > current behaviour and initialize the control list with an idmap. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/controls.h | 1 + > src/libcamera/control_serializer.cpp | 49 +++++++++++++++++++++++----- > src/libcamera/controls.cpp | 8 +++++ > 3 files changed, 49 insertions(+), 9 deletions(-) > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index 6668e4bb1053..8e5bc8b70b94 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -407,6 +407,7 @@ public: > void set(unsigned int id, const ControlValue &value); > > const ControlInfoMap *infoMap() const { return infoMap_; } > + const ControlIdMap *idMap() const { return idmap_; } > > private: > const ControlValue *find(unsigned int id) const; > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > index 08cfecca3078..d4377c024079 100644 > --- a/src/libcamera/control_serializer.cpp > +++ b/src/libcamera/control_serializer.cpp > @@ -275,6 +275,15 @@ int ControlSerializer::serialize(const ControlList &list, > infoMapHandle = 0; > } > > + const ControlIdMap *idmap = list.idMap(); > + enum ipa_controls_id_map_type idMapType; > + if (idmap == &controls::controls) > + idMapType = IPA_CONTROL_ID_MAP_CONTROLS; > + else if (idmap == &properties::properties) > + idMapType = IPA_CONTROL_ID_MAP_PROPERTIES; > + else > + idMapType = IPA_CONTROL_ID_MAP_V4L2; > + > size_t entriesSize = list.size() * sizeof(struct ipa_control_value_entry); > size_t valuesSize = 0; > for (const auto &ctrl : list) > @@ -287,6 +296,7 @@ int ControlSerializer::serialize(const ControlList &list, > hdr.entries = list.size(); > hdr.size = sizeof(hdr) + entriesSize + valuesSize; > hdr.data_offset = sizeof(hdr) + entriesSize; > + hdr.id_map_type = idMapType; > > buffer.write(&hdr); > > @@ -499,13 +509,15 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > } > > /* > - * Retrieve the ControlInfoMap associated with the ControlList based on > - * its ID. The mapping between infoMap and ID is set up when serializing > - * or deserializing ControlInfoMap. If no mapping is found (which is > - * currently the case for ControlList related to libcamera controls), > - * use the global control::control idmap. > + * Retrieve the ControlIdMap associated with the ControlList. > + * > + * The idmap is either retrieved from the list's ControlInfoMap when > + * a valid handle has been initialized at serialization time, or by > + * using the header's id_map_type field for lists that refer to the > + * globally defined libcamera controls and properties, for which no > + * ControlInfoMap is available. > */ > - const ControlInfoMap *infoMap; > + const ControlIdMap *idMap; > if (hdr->handle) { > auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(), > [&](decltype(infoMapHandles_)::value_type &entry) { > @@ -517,12 +529,31 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > return {}; > } > > - infoMap = iter->first; > + const ControlInfoMap *infoMap = iter->first; > + idMap = &infoMap->idmap(); > } else { > - infoMap = nullptr; > + switch (hdr->id_map_type) { > + case IPA_CONTROL_ID_MAP_CONTROLS: > + idMap = &controls::controls; > + break; > + > + case IPA_CONTROL_ID_MAP_PROPERTIES: > + idMap = &properties::properties; > + break; > + case IPA_CONTROL_ID_MAP_V4L2: > + LOG(Serializer, Fatal) > + << "A list of V4L2 controls requires an ControlInfoMap"; Can we have a return statement after the Fatal? (Presuming that there is no safe path forwards if Fatal is lowered to Error on Release builds). > + } > } > > - ControlList ctrls(infoMap ? infoMap->idmap() : controls::controls); > + /* > + * \todo If possible, initialize the list with the ControlInfoMap > + * so that controls can be validated against their limits. > + * Currently no validation is performed, so it's fine relying on the > + * idmap only. > + */ > + ASSERT(idMap); I presume this assert can only be hit through the IPA_CONTROL_ID_MAP_V4L2 path above? Although actually - if there's ever a new IPA_CONTROL_ID_MAP type added, it guards against that too - so the assert is good to have. Only minors there: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + ControlList ctrls(*idMap); > > for (unsigned int i = 0; i < hdr->entries; ++i) { > const struct ipa_control_value_entry *entry = > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index 5c05ba4a7cd0..96625edb1380 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -1038,6 +1038,14 @@ void ControlList::set(unsigned int id, const ControlValue &value) > * associated ControlInfoMap, nullptr is returned in that case. > */ > > +/** > + * \fn ControlList::idMap() > + * \brief Retrieve the ControlId map used to construct the ControlList > + * \return The ControlId map used to construct the ControlList. ControlsList > + * instances constructed with ControlList() have no associated idmap, nullptr is > + * returned in that case. > + */ > + > const ControlValue *ControlList::find(unsigned int id) const > { > const auto iter = controls_.find(id); >
On 01/09/2021 15:38, Jacopo Mondi wrote: > The ControlInfo class was originally designed to only transport > the control's minimum and maximum values which represent the control's > valid limits. > > Later the default value of the control has been added to the ControlInfo > class, but the control serializer implementation has not been updated > accordingly. > > This causes issues to IPA modules making use of ControlInfo::def() as, > when running in isolation, they would receive a 0 value as default. > > Fix that by serializing and deserializing the additional ControlValue > and update the protocol description accordingly. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/control_serializer.cpp | 6 ++++-- > src/libcamera/ipa_controls.cpp | 14 ++++++++------ > 2 files changed, 12 insertions(+), 8 deletions(-) > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > index 7eef041a16ee..4e8045251938 100644 > --- a/src/libcamera/control_serializer.cpp > +++ b/src/libcamera/control_serializer.cpp > @@ -105,7 +105,7 @@ size_t ControlSerializer::binarySize(const ControlValue &value) > > size_t ControlSerializer::binarySize(const ControlInfo &info) > { > - return binarySize(info.min()) + binarySize(info.max()); > + return binarySize(info.min()) + binarySize(info.max()) + binarySize(info.def()); > } > > /** > @@ -158,6 +158,7 @@ void ControlSerializer::store(const ControlInfo &info, ByteStreamBuffer &buffer) > { > store(info.min(), buffer); > store(info.max(), buffer); > + store(info.def(), buffer); > } > > /** > @@ -346,8 +347,9 @@ ControlInfo ControlSerializer::loadControlInfo(ControlType type, > > ControlValue min = loadControlValue(type, b); > ControlValue max = loadControlValue(type, b); > + ControlValue def = loadControlValue(type, b); > > - return ControlInfo(min, max); > + return ControlInfo(min, max, def); > } > > /** > diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp > index fb98cda30ede..3d172d66b30f 100644 > --- a/src/libcamera/ipa_controls.cpp > +++ b/src/libcamera/ipa_controls.cpp > @@ -108,17 +108,19 @@ > * +-------------------------+ . > * / | ... | | entry[n].offset > * | +-------------------------+ <-----´ > - * Data | | minimum value (#n) | \ > - * section | +-------------------------+ | Entry #n > - * | | maximum value (#n) | / > + * | | minimum value (#n) | \ > + * Data | +-------------------------+ | > + * section | | maximum value (#n) | | Entry #n > + * | +-------------------------+ | > + * | | default value (#n) | / > * | +-------------------------+ > * \ | ... | > * +-------------------------+ > * ~~~~ > * > - * The minimum and maximum value are stored in the platform's native data > - * format. The ipa_control_info_entry::offset field stores the offset from the > - * beginning of the data section to the info data. > + * The minimum, maximum and default value are stored in the platform's native > + * data format. The ipa_control_info_entry::offset field stores the offset from > + * the beginning of the data section to the info data. > * > * Info data in the data section shall be stored in the same order as the > * entries array, shall be aligned to a multiple of 8 bytes, and shall be >
Hi Jacopo, On Wed, Sep 01, 2021 at 04:38:00PM +0200, Jacopo Mondi wrote: > The ControlInfo class was originally designed to only transport > the control's minimum and maximum values which represent the control's > valid limits. > > Later the default value of the control has been added to the ControlInfo > class, but the control serializer implementation has not been updated > accordingly. > > This causes issues to IPA modules making use of ControlInfo::def() as, s/to/in/ > when running in isolation, they would receive a 0 value as default. s/ as default// There's no way to turn off receiving the "default" :D > > Fix that by serializing and deserializing the additional ControlValue > and update the protocol description accordingly. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/control_serializer.cpp | 6 ++++-- > src/libcamera/ipa_controls.cpp | 14 ++++++++------ > 2 files changed, 12 insertions(+), 8 deletions(-) > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > index 7eef041a16ee..4e8045251938 100644 > --- a/src/libcamera/control_serializer.cpp > +++ b/src/libcamera/control_serializer.cpp > @@ -105,7 +105,7 @@ size_t ControlSerializer::binarySize(const ControlValue &value) > > size_t ControlSerializer::binarySize(const ControlInfo &info) > { > - return binarySize(info.min()) + binarySize(info.max()); > + return binarySize(info.min()) + binarySize(info.max()) + binarySize(info.def()); > } > > /** > @@ -158,6 +158,7 @@ void ControlSerializer::store(const ControlInfo &info, ByteStreamBuffer &buffer) > { > store(info.min(), buffer); > store(info.max(), buffer); > + store(info.def(), buffer); > } > > /** > @@ -346,8 +347,9 @@ ControlInfo ControlSerializer::loadControlInfo(ControlType type, > > ControlValue min = loadControlValue(type, b); > ControlValue max = loadControlValue(type, b); > + ControlValue def = loadControlValue(type, b); > > - return ControlInfo(min, max); > + return ControlInfo(min, max, def); > } > > /** > diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp > index fb98cda30ede..3d172d66b30f 100644 > --- a/src/libcamera/ipa_controls.cpp > +++ b/src/libcamera/ipa_controls.cpp > @@ -108,17 +108,19 @@ > * +-------------------------+ . > * / | ... | | entry[n].offset > * | +-------------------------+ <-----´ > - * Data | | minimum value (#n) | \ > - * section | +-------------------------+ | Entry #n > - * | | maximum value (#n) | / > + * | | minimum value (#n) | \ > + * Data | +-------------------------+ | > + * section | | maximum value (#n) | | Entry #n > + * | +-------------------------+ | > + * | | default value (#n) | / > * | +-------------------------+ > * \ | ... | > * +-------------------------+ > * ~~~~ > * > - * The minimum and maximum value are stored in the platform's native data > - * format. The ipa_control_info_entry::offset field stores the offset from the > - * beginning of the data section to the info data. > + * The minimum, maximum and default value are stored in the platform's native s/maximum/maximum,/ s/value/values/ Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > + * data format. The ipa_control_info_entry::offset field stores the offset from > + * the beginning of the data section to the info data. > * > * Info data in the data section shall be stored in the same order as the > * entries array, shall be aligned to a multiple of 8 bytes, and shall be > -- > 2.32.0 >
Hi Jacopo On 9/1/21 8:07 PM, Jacopo Mondi wrote: > When running the IPA in isolated mode, each side of the IPC boundary > has an instance of the ControlSerializer class which is used to > serializer/deserialize controls before transmitting them on the wire. > > The IPAProxyWorker, which creates and manages the process the IPA runs into, > does not reset its ControlSerializer upon an IPA::configure() call, while > the IPAProxy does so, effectively creating a misalignment between the > two sides of the fence. > > This obviously creates issues as one side of the IPC runs with a > populated and possibly stale cache of ControlInfoMap references, while the > other side gets reset every time a new configuration is applied to the > Camera. > > Fix that by resetting the IPAProxyWorker ControlSerializer on an > IPA configure() call. > > This change fixes an issue which is easily triggered by running two > consecutive capture sessions with the IPA running in isolated mode: > ERROR Serializer control_serializer.cpp:520 Can't deserialize ControlList: unknown ControlInfoMap > > Fixes: 7832e19a599e ("utils: ipc: add templates for code generation for IPC mechanism") > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl > index 8a88bd467da7..22cbc15c5eff 100644 > --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl > @@ -79,6 +79,10 @@ public: > > {% for method in interface_main.methods %} > case {{cmd_enum_name}}::{{method.mojom_name|cap}}: { > + > +{%- if method.mojom_name == "configure" %} > + controlSerializer_.reset(); > +{%- endif %} > {{proxy_funcs.deserialize_call(method|method_param_inputs, '_ipcMessage.data()', '_ipcMessage.fds()', false, true)|indent(8, true)}} > {% for param in method|method_param_outputs %} > {{param|name}} {{param.mojom_name}};
Hi Jacopo On 9/1/21 8:07 PM, Jacopo Mondi wrote: > The IPA::configure() function has an IPAConfigInfo parameters which > contains a map of numerical indexes to ControlInfoMap instances. > > This is a leftover of the old IPA protocol, where it was not possible to > specify a rich interface as it is possible today and each entity > ControlInfoMap was indexed by a numerical id and stored in a map. > > Now that the IPA interface allows to specify parameters by name, drop the > map and send the sensor's control info map only. ack > > If we'll need more ControlInfoMap to be shared with the IPA, a new parameter > can be added to IPAConfigInfo. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/ipa/ipu3.mojom | 2 +- > src/ipa/ipu3/ipu3.cpp | 4 ++-- > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- > 3 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom > index d561c2244907..2045ce909a88 100644 > --- a/include/libcamera/ipa/ipu3.mojom > +++ b/include/libcamera/ipa/ipu3.mojom > @@ -32,7 +32,7 @@ struct IPU3Action { > > struct IPAConfigInfo { > libcamera.IPACameraSensorInfo sensorInfo; > - map<uint32, libcamera.ControlInfoMap> entityControls; > + libcamera.ControlInfoMap sensorControls; > libcamera.Size bdsOutputSize; > libcamera.Size iif; > }; > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index c925cf642611..6a74f7826332 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -337,14 +337,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize) > > int IPAIPU3::configure(const IPAConfigInfo &configInfo) > { > - if (configInfo.entityControls.empty()) { > + if (configInfo.sensorControls.empty()) { > LOG(IPAIPU3, Error) << "No controls provided"; Maybe also change the error string to be more specific now: LOG(IPAIPU3, Error) << "No sensor controls provided"; Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > return -ENODATA; > } > > sensorInfo_ = configInfo.sensorInfo; > > - ctrls_ = configInfo.entityControls.at(0); > + ctrls_ = configInfo.sensorControls; > > const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE); > if (itExp == ctrls_.end()) { > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index c287bf86e79a..92e869257e53 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -654,7 +654,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > } > > ipa::ipu3::IPAConfigInfo configInfo; > - configInfo.entityControls.emplace(0, data->cio2_.sensor()->controls()); > + configInfo.sensorControls = data->cio2_.sensor()->controls(); > configInfo.sensorInfo = sensorInfo; > configInfo.bdsOutputSize = config->imguConfig().bds; > configInfo.iif = config->imguConfig().iif;
Hi Kieran, On Thu, Sep 02, 2021 at 02:16:27PM +0100, Kieran Bingham wrote: > On 01/09/2021 15:37, Jacopo Mondi wrote: > > The IPA::configure() function has an IPAConfigInfo parameters which > > contains a map of numerical indexes to ControlInfoMap instances. > > > > This is a leftover of the old IPA protocol, where it was not possible to > > specify a rich interface as it is possible today and each entity > > ControlInfoMap was indexed by a numerical id and stored in a map. > > > > Now that the IPA interface allows to specify parameters by name, drop the > > map and send the sensor's control info map only. > > > > If we'll need more ControlInfoMap to be shared with the IPA, a new parameter > > can be added to IPAConfigInfo. > > > It looks like this will need a patch for > > https://git.libcamera.org/libcamera/ipu3-ipa.git/ > > as well. But given that it requires manually installed AIQ libraries on > non CrOS build environments, perhaps it's something myself or Umang > should handle when this series merges. I would ofc be happy if you and Umang could take care of that, but as we evolve the IPA interface on the open source IPA module, the burden of keeping the two in sync might get considerable on your side. If I'm not mistaken most the changes to the IPA interface will reflect only in ipu3.cpp from the above repository. Is there plan to merge the glue code between the PH and the closed source IPA module in libcamera ? In this way we could keep at least the interface in sync when we modify the protocol. Thanks j > > I wonder if we should be increasing the IPA ABI / version numbers too > whenever we make a change. > > But alas - it doesn't look like that's fully implemented, The mojo files > should have a version number in there (or even better, could mojo create > a hash of the interface to catch when it changes I wonder)? > > So, given that you can't update the other IPA, and you can't update the > IPU3 IPA Version (without further patches) ... I guess this is it ... > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > include/libcamera/ipa/ipu3.mojom | 2 +- > > src/ipa/ipu3/ipu3.cpp | 4 ++-- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- > > 3 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom > > index d561c2244907..2045ce909a88 100644 > > --- a/include/libcamera/ipa/ipu3.mojom > > +++ b/include/libcamera/ipa/ipu3.mojom > > @@ -32,7 +32,7 @@ struct IPU3Action { > > > > struct IPAConfigInfo { > > libcamera.IPACameraSensorInfo sensorInfo; > > - map<uint32, libcamera.ControlInfoMap> entityControls; > > + libcamera.ControlInfoMap sensorControls; > > libcamera.Size bdsOutputSize; > > libcamera.Size iif; > > }; > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > > index c925cf642611..6a74f7826332 100644 > > --- a/src/ipa/ipu3/ipu3.cpp > > +++ b/src/ipa/ipu3/ipu3.cpp > > @@ -337,14 +337,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize) > > > > int IPAIPU3::configure(const IPAConfigInfo &configInfo) > > { > > - if (configInfo.entityControls.empty()) { > > + if (configInfo.sensorControls.empty()) { > > LOG(IPAIPU3, Error) << "No controls provided"; > > return -ENODATA; > > } > > > > sensorInfo_ = configInfo.sensorInfo; > > > > - ctrls_ = configInfo.entityControls.at(0); > > + ctrls_ = configInfo.sensorControls; > > > > const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE); > > if (itExp == ctrls_.end()) { > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index c287bf86e79a..92e869257e53 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -654,7 +654,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > > } > > > > ipa::ipu3::IPAConfigInfo configInfo; > > - configInfo.entityControls.emplace(0, data->cio2_.sensor()->controls()); > > + configInfo.sensorControls = data->cio2_.sensor()->controls(); > > configInfo.sensorInfo = sensorInfo; > > configInfo.bdsOutputSize = config->imguConfig().bds; > > configInfo.iif = config->imguConfig().iif; > >
Hi Kieran, On Thu, Sep 02, 2021 at 03:05:23PM +0100, Kieran Bingham wrote: > On 01/09/2021 15:37, Jacopo Mondi wrote: > > When running the IPA in isolated mode, each side of the IPC boundary > > has an instance of the ControlSerializer class which is used to > > serializer/deserialize controls before transmitting them on the wire. > > > > The IPAProxyWorker, which creates and manages the process the IPA runs into, > > does not reset its ControlSerializer upon an IPA::configure() call, while > > the IPAProxy does so, effectively creating a misalignment between the > > two sides of the fence. > > > > This obviously creates issues as one side of the IPC runs with a > > populated and possibly stale cache of ControlInfoMap references, while the > > other side gets reset every time a new configuration is applied to the > > Camera. > > > > Fix that by resetting the IPAProxyWorker ControlSerializer on an > > IPA configure() call. > > > > This change fixes an issue which is easily triggered by running two > > consecutive capture sessions with the IPA running in isolated mode: > > ERROR Serializer control_serializer.cpp:520 Can't deserialize ControlList: unknown ControlInfoMap > > > > Fixes: 7832e19a599e ("utils: ipc: add templates for code generation for IPC mechanism") > > Is it also technically fixing the patch that added the reset on the > other side? as that's when the misalignment begins? > The 'other side' was reset from the very beginning. To be honest I would question the usage of Fixes in libcamera, as we don't have versions to backport to, but I added this mosotly for cargo cult. Iff we want to keep using Fixes I think that's the opportune commit to point to, as this patch should be ideally fixed-up in that one (if we could travel back in time) > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl > > index 8a88bd467da7..22cbc15c5eff 100644 > > --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl > > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl > > @@ -79,6 +79,10 @@ public: > > > > {% for method in interface_main.methods %} > > case {{cmd_enum_name}}::{{method.mojom_name|cap}}: { > > + > > +{%- if method.mojom_name == "configure" %} > > + controlSerializer_.reset(); > > one more level indentation >> ? > > It looks like we're "inside" this case statement, so it should be one > level deeper. > Looking at the generated code case _IPU3Cmd::Configure: { controlSerializer_.reset(); const size_t configInfoStart = 0; IPAConfigInfo configInfo = IPADataSerializer<IPAConfigInfo>::deserialize( You are correct the indentation is wrong, but it does anyway matches the rest of the file :) > > > +{%- endif %} > > > Wow, these templates are hard to parse :-) > > But ... it looks like this is right ;-) > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > {{proxy_funcs.deserialize_call(method|method_param_inputs, '_ipcMessage.data()', '_ipcMessage.fds()', false, true)|indent(8, true)}} > > {% for param in method|method_param_outputs %} > > {{param|name}} {{param.mojom_name}}; > >
Hi Kieran On Thu, Sep 02, 2021 at 03:34:01PM +0100, Kieran Bingham wrote: > On 01/09/2021 15:37, Jacopo Mondi wrote: > > When a ControlList is deserialized, the code searches for a valid > > ControlInfoMap in the local cache and use its id map to initialize the > > list. If no valid ControlInfoMap is found, as it's usually the case > > for lists transporting libcamera controls and properties, the globally > > defined controls::controls id map is used unconditionally. > > > > This breaks the deserialization of libcamera properties, for which a > > wrong idmap is used at construction time. > > > > As the serialization header now transports an id_map_type field, store > > the idmap type at serialization time, and re-use it at > > deserialization time to identify the correct id map. > > > > Also make the validation stricter by imposing to list of V4L2 controls to > > have an associated ControlInfoMap available, as there is no globally > > defined idmap for such controls. > > > > To be able to retrieve the idmap associated with a ControlList, add an > > accessor function to the ControlList class. > > > > It might be worth in future using a ControlInfoMap to initialize the > > deserialized ControlList to implement controls validation against their > > limit. As such validation is not implemented at the moment, maintain the > > current behaviour and initialize the control list with an idmap. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > include/libcamera/controls.h | 1 + > > src/libcamera/control_serializer.cpp | 49 +++++++++++++++++++++++----- > > src/libcamera/controls.cpp | 8 +++++ > > 3 files changed, 49 insertions(+), 9 deletions(-) > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > index 6668e4bb1053..8e5bc8b70b94 100644 > > --- a/include/libcamera/controls.h > > +++ b/include/libcamera/controls.h > > @@ -407,6 +407,7 @@ public: > > void set(unsigned int id, const ControlValue &value); > > > > const ControlInfoMap *infoMap() const { return infoMap_; } > > + const ControlIdMap *idMap() const { return idmap_; } > > > > private: > > const ControlValue *find(unsigned int id) const; > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > > index 08cfecca3078..d4377c024079 100644 > > --- a/src/libcamera/control_serializer.cpp > > +++ b/src/libcamera/control_serializer.cpp > > @@ -275,6 +275,15 @@ int ControlSerializer::serialize(const ControlList &list, > > infoMapHandle = 0; > > } > > > > + const ControlIdMap *idmap = list.idMap(); > > + enum ipa_controls_id_map_type idMapType; > > + if (idmap == &controls::controls) > > + idMapType = IPA_CONTROL_ID_MAP_CONTROLS; > > + else if (idmap == &properties::properties) > > + idMapType = IPA_CONTROL_ID_MAP_PROPERTIES; > > + else > > + idMapType = IPA_CONTROL_ID_MAP_V4L2; > > + > > size_t entriesSize = list.size() * sizeof(struct ipa_control_value_entry); > > size_t valuesSize = 0; > > for (const auto &ctrl : list) > > @@ -287,6 +296,7 @@ int ControlSerializer::serialize(const ControlList &list, > > hdr.entries = list.size(); > > hdr.size = sizeof(hdr) + entriesSize + valuesSize; > > hdr.data_offset = sizeof(hdr) + entriesSize; > > + hdr.id_map_type = idMapType; > > > > buffer.write(&hdr); > > > > @@ -499,13 +509,15 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > > } > > > > /* > > - * Retrieve the ControlInfoMap associated with the ControlList based on > > - * its ID. The mapping between infoMap and ID is set up when serializing > > - * or deserializing ControlInfoMap. If no mapping is found (which is > > - * currently the case for ControlList related to libcamera controls), > > - * use the global control::control idmap. > > + * Retrieve the ControlIdMap associated with the ControlList. > > + * > > + * The idmap is either retrieved from the list's ControlInfoMap when > > + * a valid handle has been initialized at serialization time, or by > > + * using the header's id_map_type field for lists that refer to the > > + * globally defined libcamera controls and properties, for which no > > + * ControlInfoMap is available. > > */ > > - const ControlInfoMap *infoMap; > > + const ControlIdMap *idMap; > > if (hdr->handle) { > > auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(), > > [&](decltype(infoMapHandles_)::value_type &entry) { > > @@ -517,12 +529,31 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > > return {}; > > } > > > > - infoMap = iter->first; > > + const ControlInfoMap *infoMap = iter->first; > > + idMap = &infoMap->idmap(); > > } else { > > - infoMap = nullptr; > > + switch (hdr->id_map_type) { > > + case IPA_CONTROL_ID_MAP_CONTROLS: > > + idMap = &controls::controls; > > + break; > > + > > + case IPA_CONTROL_ID_MAP_PROPERTIES: > > + idMap = &properties::properties; > > + break; Ups, missing a line break here! > > + case IPA_CONTROL_ID_MAP_V4L2: > > + LOG(Serializer, Fatal) > > + << "A list of V4L2 controls requires an ControlInfoMap"; > > Can we have a return statement after the Fatal? > > (Presuming that there is no safe path forwards if Fatal is lowered to > Error on Release builds). > Right! In production builds this won't exit. I'll add a return > > > + } > > } > > > > - ControlList ctrls(infoMap ? infoMap->idmap() : controls::controls); > > + /* > > + * \todo If possible, initialize the list with the ControlInfoMap > > + * so that controls can be validated against their limits. > > + * Currently no validation is performed, so it's fine relying on the > > + * idmap only. > > + */ > > + ASSERT(idMap); > > I presume this assert can only be hit through the > IPA_CONTROL_ID_MAP_V4L2 path above? It could actually be hit through the if (hdr->handle) branch where the idMap is retrieved from the infoMap > > Although actually - if there's ever a new IPA_CONTROL_ID_MAP type added, > it guards against that too - so the assert is good to have. If we add a new IPA_CONTROL_ID_MAP type the compiler will warn that not all the cases are handled in the switch statment, as I didn't add a default: case precisely for that reason. So yes, I could move the ASSERT() in the if() {} branch actually. > > Only minors there: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > + ControlList ctrls(*idMap); > > > > for (unsigned int i = 0; i < hdr->entries; ++i) { > > const struct ipa_control_value_entry *entry = > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > index 5c05ba4a7cd0..96625edb1380 100644 > > --- a/src/libcamera/controls.cpp > > +++ b/src/libcamera/controls.cpp > > @@ -1038,6 +1038,14 @@ void ControlList::set(unsigned int id, const ControlValue &value) > > * associated ControlInfoMap, nullptr is returned in that case. > > */ > > > > +/** > > + * \fn ControlList::idMap() > > + * \brief Retrieve the ControlId map used to construct the ControlList > > + * \return The ControlId map used to construct the ControlList. ControlsList > > + * instances constructed with ControlList() have no associated idmap, nullptr is > > + * returned in that case. > > + */ > > + > > const ControlValue *ControlList::find(unsigned int id) const > > { > > const auto iter = controls_.find(id); > >
Hi Jacopo, On Wed, Sep 01, 2021 at 04:37:55PM +0200, Jacopo Mondi wrote: > The IPA::configure() function has an IPAConfigInfo parameters which > contains a map of numerical indexes to ControlInfoMap instances. > > This is a leftover of the old IPA protocol, where it was not possible to > specify a rich interface as it is possible today and each entity > ControlInfoMap was indexed by a numerical id and stored in a map. > > Now that the IPA interface allows to specify parameters by name, drop the > map and send the sensor's control info map only. > > If we'll need more ControlInfoMap to be shared with the IPA, a new parameter > can be added to IPAConfigInfo. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Looks good. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > include/libcamera/ipa/ipu3.mojom | 2 +- > src/ipa/ipu3/ipu3.cpp | 4 ++-- > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- > 3 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom > index d561c2244907..2045ce909a88 100644 > --- a/include/libcamera/ipa/ipu3.mojom > +++ b/include/libcamera/ipa/ipu3.mojom > @@ -32,7 +32,7 @@ struct IPU3Action { > > struct IPAConfigInfo { > libcamera.IPACameraSensorInfo sensorInfo; > - map<uint32, libcamera.ControlInfoMap> entityControls; > + libcamera.ControlInfoMap sensorControls; > libcamera.Size bdsOutputSize; > libcamera.Size iif; > }; > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index c925cf642611..6a74f7826332 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -337,14 +337,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize) > > int IPAIPU3::configure(const IPAConfigInfo &configInfo) > { > - if (configInfo.entityControls.empty()) { > + if (configInfo.sensorControls.empty()) { > LOG(IPAIPU3, Error) << "No controls provided"; > return -ENODATA; > } > > sensorInfo_ = configInfo.sensorInfo; > > - ctrls_ = configInfo.entityControls.at(0); > + ctrls_ = configInfo.sensorControls; > > const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE); > if (itExp == ctrls_.end()) { > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index c287bf86e79a..92e869257e53 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -654,7 +654,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > } > > ipa::ipu3::IPAConfigInfo configInfo; > - configInfo.entityControls.emplace(0, data->cio2_.sensor()->controls()); > + configInfo.sensorControls = data->cio2_.sensor()->controls(); > configInfo.sensorInfo = sensorInfo; > configInfo.bdsOutputSize = config->imguConfig().bds; > configInfo.iif = config->imguConfig().iif; > -- > 2.32.0 >
Hi Kieran, On Thu, Sep 02, 2021 at 02:16:27PM +0100, Kieran Bingham wrote: > On 01/09/2021 15:37, Jacopo Mondi wrote: > > The IPA::configure() function has an IPAConfigInfo parameters which > > contains a map of numerical indexes to ControlInfoMap instances. > > > > This is a leftover of the old IPA protocol, where it was not possible to > > specify a rich interface as it is possible today and each entity > > ControlInfoMap was indexed by a numerical id and stored in a map. > > > > Now that the IPA interface allows to specify parameters by name, drop the > > map and send the sensor's control info map only. > > > > If we'll need more ControlInfoMap to be shared with the IPA, a new parameter > > can be added to IPAConfigInfo. > > > It looks like this will need a patch for > > https://git.libcamera.org/libcamera/ipu3-ipa.git/ > > as well. But given that it requires manually installed AIQ libraries on > non CrOS build environments, perhaps it's something myself or Umang > should handle when this series merges. > > I wonder if we should be increasing the IPA ABI / version numbers too > whenever we make a change. We don't have a release yet so it should be fine, right? :p I guess we gotta start working on this for real... I'll prepare a summary + discussion points. Paul > > But alas - it doesn't look like that's fully implemented, The mojo files > should have a version number in there (or even better, could mojo create > a hash of the interface to catch when it changes I wonder)? > > So, given that you can't update the other IPA, and you can't update the > IPU3 IPA Version (without further patches) ... I guess this is it ... > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > include/libcamera/ipa/ipu3.mojom | 2 +- > > src/ipa/ipu3/ipu3.cpp | 4 ++-- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- > > 3 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom > > index d561c2244907..2045ce909a88 100644 > > --- a/include/libcamera/ipa/ipu3.mojom > > +++ b/include/libcamera/ipa/ipu3.mojom > > @@ -32,7 +32,7 @@ struct IPU3Action { > > > > struct IPAConfigInfo { > > libcamera.IPACameraSensorInfo sensorInfo; > > - map<uint32, libcamera.ControlInfoMap> entityControls; > > + libcamera.ControlInfoMap sensorControls; > > libcamera.Size bdsOutputSize; > > libcamera.Size iif; > > }; > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > > index c925cf642611..6a74f7826332 100644 > > --- a/src/ipa/ipu3/ipu3.cpp > > +++ b/src/ipa/ipu3/ipu3.cpp > > @@ -337,14 +337,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize) > > > > int IPAIPU3::configure(const IPAConfigInfo &configInfo) > > { > > - if (configInfo.entityControls.empty()) { > > + if (configInfo.sensorControls.empty()) { > > LOG(IPAIPU3, Error) << "No controls provided"; > > return -ENODATA; > > } > > > > sensorInfo_ = configInfo.sensorInfo; > > > > - ctrls_ = configInfo.entityControls.at(0); > > + ctrls_ = configInfo.sensorControls; > > > > const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE); > > if (itExp == ctrls_.end()) { > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index c287bf86e79a..92e869257e53 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -654,7 +654,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > > } > > > > ipa::ipu3::IPAConfigInfo configInfo; > > - configInfo.entityControls.emplace(0, data->cio2_.sensor()->controls()); > > + configInfo.sensorControls = data->cio2_.sensor()->controls(); > > configInfo.sensorInfo = sensorInfo; > > configInfo.bdsOutputSize = config->imguConfig().bds; > > configInfo.iif = config->imguConfig().iif; > >
Hi Jacopo, On Wed, Sep 01, 2021 at 04:37:56PM +0200, Jacopo Mondi wrote: > When running the IPA in isolated mode, each side of the IPC boundary > has an instance of the ControlSerializer class which is used to > serializer/deserialize controls before transmitting them on the wire. > > The IPAProxyWorker, which creates and manages the process the IPA runs into, s/into/in/ > does not reset its ControlSerializer upon an IPA::configure() call, while > the IPAProxy does so, effectively creating a misalignment between the s/ so// > two sides of the fence. > > This obviously creates issues as one side of the IPC runs with a > populated and possibly stale cache of ControlInfoMap references, while the > other side gets reset every time a new configuration is applied to the > Camera. > > Fix that by resetting the IPAProxyWorker ControlSerializer on an > IPA configure() call. > > This change fixes an issue which is easily triggered by running two s/ / / > consecutive capture sessions with the IPA running in isolated mode: > ERROR Serializer control_serializer.cpp:520 Can't deserialize ControlList: unknown ControlInfoMap > > Fixes: 7832e19a599e ("utils: ipc: add templates for code generation for IPC mechanism") > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Looks good. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl > index 8a88bd467da7..22cbc15c5eff 100644 > --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl > @@ -79,6 +79,10 @@ public: > > {% for method in interface_main.methods %} > case {{cmd_enum_name}}::{{method.mojom_name|cap}}: { > + > +{%- if method.mojom_name == "configure" %} > + controlSerializer_.reset(); > +{%- endif %} > {{proxy_funcs.deserialize_call(method|method_param_inputs, '_ipcMessage.data()', '_ipcMessage.fds()', false, true)|indent(8, true)}} > {% for param in method|method_param_outputs %} > {{param|name}} {{param.mojom_name}}; > -- > 2.32.0 >
Hi Kieran, On Thu, Sep 02, 2021 at 03:05:23PM +0100, Kieran Bingham wrote: > On 01/09/2021 15:37, Jacopo Mondi wrote: > > When running the IPA in isolated mode, each side of the IPC boundary > > has an instance of the ControlSerializer class which is used to > > serializer/deserialize controls before transmitting them on the wire. > > > > The IPAProxyWorker, which creates and manages the process the IPA runs into, > > does not reset its ControlSerializer upon an IPA::configure() call, while > > the IPAProxy does so, effectively creating a misalignment between the > > two sides of the fence. > > > > This obviously creates issues as one side of the IPC runs with a > > populated and possibly stale cache of ControlInfoMap references, while the > > other side gets reset every time a new configuration is applied to the > > Camera. > > > > Fix that by resetting the IPAProxyWorker ControlSerializer on an > > IPA configure() call. > > > > This change fixes an issue which is easily triggered by running two > > consecutive capture sessions with the IPA running in isolated mode: > > ERROR Serializer control_serializer.cpp:520 Can't deserialize ControlList: unknown ControlInfoMap > > > > Fixes: 7832e19a599e ("utils: ipc: add templates for code generation for IPC mechanism") > > Is it also technically fixing the patch that added the reset on the > other side? as that's when the misalignment begins? > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl > > index 8a88bd467da7..22cbc15c5eff 100644 > > --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl > > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl > > @@ -79,6 +79,10 @@ public: > > > > {% for method in interface_main.methods %} > > case {{cmd_enum_name}}::{{method.mojom_name|cap}}: { > > + > > +{%- if method.mojom_name == "configure" %} > > + controlSerializer_.reset(); > > one more level indentation >> ? Yeah, that one line should be indented one more in to align in the case statement. Paul > > It looks like we're "inside" this case statement, so it should be one > level deeper. > > > > +{%- endif %} > > > Wow, these templates are hard to parse :-) > > But ... it looks like this is right ;-) > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > {{proxy_funcs.deserialize_call(method|method_param_inputs, '_ipcMessage.data()', '_ipcMessage.fds()', false, true)|indent(8, true)}} > > {% for param in method|method_param_outputs %} > > {{param|name}} {{param.mojom_name}}; > >
Hi Jacopo, On Wed, Sep 01, 2021 at 04:37:57PM +0200, Jacopo Mondi wrote: > When running the IPA in isolated mode, each side of the IPC boundary > has an instance of the ControlSerializer class. > > Each serializer instance tags with a numerical id (handle) each > ControlInfoMap instance it serializes, to be capable of associating > ControlList with it at serialization and deserialization time, and > increments the numerical counter for each newly serialized control info > map. > > Having two instances of the ControlSerializer class running in two > different process spaces, each instance increments its own counter, > preventing from maintaining a globally shared state where each > ControlInfoMap instance is reference by a unique identifier. > > Fix this by advancing the serial_ counter at ControlInfoMap > de-serialization time, so that each newly serialized map will have a > globally unique identifier. > > Fixes: 2c5f0ad23aa4 ("libcamera: Add controls serializer") > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/libcamera/control_serializer.cpp | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > index 27360526f9eb..08cfecca3078 100644 > --- a/src/libcamera/control_serializer.cpp > +++ b/src/libcamera/control_serializer.cpp > @@ -380,6 +380,9 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > return {}; > } > > + /* Keep the info map handles in sync between the two sides of IPC. */ > + serial_ = std::max(serial_, hdr->handle); > + > /* > * Use the ControlIdMap corresponding to the id map type. If the type > * references a globally defined id map (such as controls::controls > -- > 2.32.0 >
Hi Jacopo, On Wed, Sep 01, 2021 at 04:37:58PM +0200, Jacopo Mondi wrote: > When a ControlList is deserialized, the code searches for a valid > ControlInfoMap in the local cache and use its id map to initialize the > list. If no valid ControlInfoMap is found, as it's usually the case > for lists transporting libcamera controls and properties, the globally > defined controls::controls id map is used unconditionally. > > This breaks the deserialization of libcamera properties, for which a > wrong idmap is used at construction time. > > As the serialization header now transports an id_map_type field, store > the idmap type at serialization time, and re-use it at > deserialization time to identify the correct id map. > > Also make the validation stricter by imposing to list of V4L2 controls to > have an associated ControlInfoMap available, as there is no globally > defined idmap for such controls. > > To be able to retrieve the idmap associated with a ControlList, add an > accessor function to the ControlList class. > > It might be worth in future using a ControlInfoMap to initialize the > deserialized ControlList to implement controls validation against their > limit. As such validation is not implemented at the moment, maintain the > current behaviour and initialize the control list with an idmap. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > include/libcamera/controls.h | 1 + > src/libcamera/control_serializer.cpp | 49 +++++++++++++++++++++++----- > src/libcamera/controls.cpp | 8 +++++ > 3 files changed, 49 insertions(+), 9 deletions(-) > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index 6668e4bb1053..8e5bc8b70b94 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -407,6 +407,7 @@ public: > void set(unsigned int id, const ControlValue &value); > > const ControlInfoMap *infoMap() const { return infoMap_; } > + const ControlIdMap *idMap() const { return idmap_; } > > private: > const ControlValue *find(unsigned int id) const; > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > index 08cfecca3078..d4377c024079 100644 > --- a/src/libcamera/control_serializer.cpp > +++ b/src/libcamera/control_serializer.cpp > @@ -275,6 +275,15 @@ int ControlSerializer::serialize(const ControlList &list, > infoMapHandle = 0; > } > > + const ControlIdMap *idmap = list.idMap(); > + enum ipa_controls_id_map_type idMapType; > + if (idmap == &controls::controls) > + idMapType = IPA_CONTROL_ID_MAP_CONTROLS; > + else if (idmap == &properties::properties) > + idMapType = IPA_CONTROL_ID_MAP_PROPERTIES; > + else > + idMapType = IPA_CONTROL_ID_MAP_V4L2; > + > size_t entriesSize = list.size() * sizeof(struct ipa_control_value_entry); > size_t valuesSize = 0; > for (const auto &ctrl : list) > @@ -287,6 +296,7 @@ int ControlSerializer::serialize(const ControlList &list, > hdr.entries = list.size(); > hdr.size = sizeof(hdr) + entriesSize + valuesSize; > hdr.data_offset = sizeof(hdr) + entriesSize; > + hdr.id_map_type = idMapType; > > buffer.write(&hdr); > > @@ -499,13 +509,15 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > } > > /* > - * Retrieve the ControlInfoMap associated with the ControlList based on > - * its ID. The mapping between infoMap and ID is set up when serializing > - * or deserializing ControlInfoMap. If no mapping is found (which is > - * currently the case for ControlList related to libcamera controls), > - * use the global control::control idmap. > + * Retrieve the ControlIdMap associated with the ControlList. > + * > + * The idmap is either retrieved from the list's ControlInfoMap when > + * a valid handle has been initialized at serialization time, or by > + * using the header's id_map_type field for lists that refer to the > + * globally defined libcamera controls and properties, for which no > + * ControlInfoMap is available. > */ > - const ControlInfoMap *infoMap; > + const ControlIdMap *idMap; > if (hdr->handle) { > auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(), > [&](decltype(infoMapHandles_)::value_type &entry) { > @@ -517,12 +529,31 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > return {}; > } > > - infoMap = iter->first; > + const ControlInfoMap *infoMap = iter->first; > + idMap = &infoMap->idmap(); > } else { > - infoMap = nullptr; > + switch (hdr->id_map_type) { > + case IPA_CONTROL_ID_MAP_CONTROLS: > + idMap = &controls::controls; > + break; > + > + case IPA_CONTROL_ID_MAP_PROPERTIES: > + idMap = &properties::properties; > + break; > + case IPA_CONTROL_ID_MAP_V4L2: > + LOG(Serializer, Fatal) > + << "A list of V4L2 controls requires an ControlInfoMap"; > + } > } > > - ControlList ctrls(infoMap ? infoMap->idmap() : controls::controls); > + /* > + * \todo If possible, initialize the list with the ControlInfoMap > + * so that controls can be validated against their limits. > + * Currently no validation is performed, so it's fine relying on the > + * idmap only. > + */ > + ASSERT(idMap); > + ControlList ctrls(*idMap); > > for (unsigned int i = 0; i < hdr->entries; ++i) { > const struct ipa_control_value_entry *entry = > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index 5c05ba4a7cd0..96625edb1380 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -1038,6 +1038,14 @@ void ControlList::set(unsigned int id, const ControlValue &value) > * associated ControlInfoMap, nullptr is returned in that case. > */ > > +/** > + * \fn ControlList::idMap() > + * \brief Retrieve the ControlId map used to construct the ControlList > + * \return The ControlId map used to construct the ControlList. ControlsList > + * instances constructed with ControlList() have no associated idmap, nullptr is > + * returned in that case. > + */ > + > const ControlValue *ControlList::find(unsigned int id) const > { > const auto iter = controls_.find(id); > -- > 2.32.0 >
On Fri, Sep 03, 2021 at 06:33:30PM +0900, paul.elder@ideasonboard.com wrote: > On Thu, Sep 02, 2021 at 02:16:27PM +0100, Kieran Bingham wrote: > > On 01/09/2021 15:37, Jacopo Mondi wrote: > > > The IPA::configure() function has an IPAConfigInfo parameters which > > > contains a map of numerical indexes to ControlInfoMap instances. > > > > > > This is a leftover of the old IPA protocol, where it was not possible to > > > specify a rich interface as it is possible today and each entity > > > ControlInfoMap was indexed by a numerical id and stored in a map. > > > > > > Now that the IPA interface allows to specify parameters by name, drop the > > > map and send the sensor's control info map only. > > > > > > If we'll need more ControlInfoMap to be shared with the IPA, a new parameter > > > can be added to IPAConfigInfo. > > > > > > It looks like this will need a patch for > > > > https://git.libcamera.org/libcamera/ipu3-ipa.git/ > > > > as well. But given that it requires manually installed AIQ libraries on > > non CrOS build environments, perhaps it's something myself or Umang > > should handle when this series merges. > > > > I wonder if we should be increasing the IPA ABI / version numbers too > > whenever we make a change. > > We don't have a release yet so it should be fine, right? :p > > I guess we gotta start working on this for real... I'll prepare > a summary + discussion points. Another can of worms :-) It certainly needs to be addressed, but will need to include .mojom versioning, and protocol stability. We can start discussions, but I don't think the implementation is the priority at this point. > > But alas - it doesn't look like that's fully implemented, The mojo files > > should have a version number in there (or even better, could mojo create > > a hash of the interface to catch when it changes I wonder)? > > > > So, given that you can't update the other IPA, and you can't update the > > IPU3 IPA Version (without further patches) ... I guess this is it ... > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > include/libcamera/ipa/ipu3.mojom | 2 +- > > > src/ipa/ipu3/ipu3.cpp | 4 ++-- > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- > > > 3 files changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom > > > index d561c2244907..2045ce909a88 100644 > > > --- a/include/libcamera/ipa/ipu3.mojom > > > +++ b/include/libcamera/ipa/ipu3.mojom > > > @@ -32,7 +32,7 @@ struct IPU3Action { > > > > > > struct IPAConfigInfo { > > > libcamera.IPACameraSensorInfo sensorInfo; > > > - map<uint32, libcamera.ControlInfoMap> entityControls; > > > + libcamera.ControlInfoMap sensorControls; > > > libcamera.Size bdsOutputSize; > > > libcamera.Size iif; > > > }; > > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > > > index c925cf642611..6a74f7826332 100644 > > > --- a/src/ipa/ipu3/ipu3.cpp > > > +++ b/src/ipa/ipu3/ipu3.cpp > > > @@ -337,14 +337,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize) > > > > > > int IPAIPU3::configure(const IPAConfigInfo &configInfo) > > > { > > > - if (configInfo.entityControls.empty()) { > > > + if (configInfo.sensorControls.empty()) { > > > LOG(IPAIPU3, Error) << "No controls provided"; > > > return -ENODATA; > > > } > > > > > > sensorInfo_ = configInfo.sensorInfo; > > > > > > - ctrls_ = configInfo.entityControls.at(0); > > > + ctrls_ = configInfo.sensorControls; > > > > > > const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE); > > > if (itExp == ctrls_.end()) { > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > index c287bf86e79a..92e869257e53 100644 > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > @@ -654,7 +654,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > > > } > > > > > > ipa::ipu3::IPAConfigInfo configInfo; > > > - configInfo.entityControls.emplace(0, data->cio2_.sensor()->controls()); > > > + configInfo.sensorControls = data->cio2_.sensor()->controls(); > > > configInfo.sensorInfo = sensorInfo; > > > configInfo.bdsOutputSize = config->imguConfig().bds; > > > configInfo.iif = config->imguConfig().iif;
Hi Jacopo, On Fri, Sep 03, 2021 at 10:57:32AM +0200, Jacopo Mondi wrote: > On Thu, Sep 02, 2021 at 02:16:27PM +0100, Kieran Bingham wrote: > > On 01/09/2021 15:37, Jacopo Mondi wrote: > > > The IPA::configure() function has an IPAConfigInfo parameters which > > > contains a map of numerical indexes to ControlInfoMap instances. > > > > > > This is a leftover of the old IPA protocol, where it was not possible to > > > specify a rich interface as it is possible today and each entity > > > ControlInfoMap was indexed by a numerical id and stored in a map. > > > > > > Now that the IPA interface allows to specify parameters by name, drop the > > > map and send the sensor's control info map only. > > > > > > If we'll need more ControlInfoMap to be shared with the IPA, a new parameter > > > can be added to IPAConfigInfo. > > > > It looks like this will need a patch for > > > > https://git.libcamera.org/libcamera/ipu3-ipa.git/ > > > > as well. But given that it requires manually installed AIQ libraries on > > non CrOS build environments, perhaps it's something myself or Umang > > should handle when this series merges. > > I would ofc be happy if you and Umang could take care of that, but as > we evolve the IPA interface on the open source IPA module, the burden > of keeping the two in sync might get considerable on your side. > > If I'm not mistaken most the changes to the IPA interface will reflect > only in ipu3.cpp from the above repository. > > Is there plan to merge the glue code between the PH and the closed > source IPA module in libcamera ? In this way we could keep > at least the interface in sync when we modify the protocol. No, closed-source implementations won't be merged in libcamera. This is a temporary issue until we stabilize the ABI, after that we'll need versioning (and of course we'll also need to carefully design v1 to make sure a v2 will never be needed ;-)). > > I wonder if we should be increasing the IPA ABI / version numbers too > > whenever we make a change. > > > > But alas - it doesn't look like that's fully implemented, The mojo files > > should have a version number in there (or even better, could mojo create > > a hash of the interface to catch when it changes I wonder)? > > > > So, given that you can't update the other IPA, and you can't update the > > IPU3 IPA Version (without further patches) ... I guess this is it ... > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > include/libcamera/ipa/ipu3.mojom | 2 +- > > > src/ipa/ipu3/ipu3.cpp | 4 ++-- > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- > > > 3 files changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom > > > index d561c2244907..2045ce909a88 100644 > > > --- a/include/libcamera/ipa/ipu3.mojom > > > +++ b/include/libcamera/ipa/ipu3.mojom > > > @@ -32,7 +32,7 @@ struct IPU3Action { > > > > > > struct IPAConfigInfo { > > > libcamera.IPACameraSensorInfo sensorInfo; > > > - map<uint32, libcamera.ControlInfoMap> entityControls; > > > + libcamera.ControlInfoMap sensorControls; > > > libcamera.Size bdsOutputSize; > > > libcamera.Size iif; > > > }; > > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > > > index c925cf642611..6a74f7826332 100644 > > > --- a/src/ipa/ipu3/ipu3.cpp > > > +++ b/src/ipa/ipu3/ipu3.cpp > > > @@ -337,14 +337,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize) > > > > > > int IPAIPU3::configure(const IPAConfigInfo &configInfo) > > > { > > > - if (configInfo.entityControls.empty()) { > > > + if (configInfo.sensorControls.empty()) { > > > LOG(IPAIPU3, Error) << "No controls provided"; > > > return -ENODATA; > > > } > > > > > > sensorInfo_ = configInfo.sensorInfo; > > > > > > - ctrls_ = configInfo.entityControls.at(0); > > > + ctrls_ = configInfo.sensorControls; > > > > > > const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE); > > > if (itExp == ctrls_.end()) { > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > index c287bf86e79a..92e869257e53 100644 > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > @@ -654,7 +654,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > > > } > > > > > > ipa::ipu3::IPAConfigInfo configInfo; > > > - configInfo.entityControls.emplace(0, data->cio2_.sensor()->controls()); > > > + configInfo.sensorControls = data->cio2_.sensor()->controls(); > > > configInfo.sensorInfo = sensorInfo; > > > configInfo.bdsOutputSize = config->imguConfig().bds; > > > configInfo.iif = config->imguConfig().iif;
Hi Jacopo, Thank you for the patch. On Wed, Sep 01, 2021 at 04:37:55PM +0200, Jacopo Mondi wrote: > The IPA::configure() function has an IPAConfigInfo parameters which > contains a map of numerical indexes to ControlInfoMap instances. > > This is a leftover of the old IPA protocol, where it was not possible to > specify a rich interface as it is possible today and each entity > ControlInfoMap was indexed by a numerical id and stored in a map. > > Now that the IPA interface allows to specify parameters by name, drop the > map and send the sensor's control info map only. > > If we'll need more ControlInfoMap to be shared with the IPA, a new parameter > can be added to IPAConfigInfo. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/ipa/ipu3.mojom | 2 +- > src/ipa/ipu3/ipu3.cpp | 4 ++-- > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- > 3 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom > index d561c2244907..2045ce909a88 100644 > --- a/include/libcamera/ipa/ipu3.mojom > +++ b/include/libcamera/ipa/ipu3.mojom > @@ -32,7 +32,7 @@ struct IPU3Action { > > struct IPAConfigInfo { > libcamera.IPACameraSensorInfo sensorInfo; > - map<uint32, libcamera.ControlInfoMap> entityControls; > + libcamera.ControlInfoMap sensorControls; > libcamera.Size bdsOutputSize; > libcamera.Size iif; > }; > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index c925cf642611..6a74f7826332 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -337,14 +337,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize) > > int IPAIPU3::configure(const IPAConfigInfo &configInfo) > { > - if (configInfo.entityControls.empty()) { > + if (configInfo.sensorControls.empty()) { > LOG(IPAIPU3, Error) << "No controls provided"; > return -ENODATA; > } > > sensorInfo_ = configInfo.sensorInfo; > > - ctrls_ = configInfo.entityControls.at(0); > + ctrls_ = configInfo.sensorControls; > > const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE); > if (itExp == ctrls_.end()) { > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index c287bf86e79a..92e869257e53 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -654,7 +654,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > } > > ipa::ipu3::IPAConfigInfo configInfo; > - configInfo.entityControls.emplace(0, data->cio2_.sensor()->controls()); > + configInfo.sensorControls = data->cio2_.sensor()->controls(); > configInfo.sensorInfo = sensorInfo; > configInfo.bdsOutputSize = config->imguConfig().bds; > configInfo.iif = config->imguConfig().iif;
Hi Jacopo, Thank you for the patch. On Wed, Sep 01, 2021 at 04:37:56PM +0200, Jacopo Mondi wrote: > When running the IPA in isolated mode, each side of the IPC boundary > has an instance of the ControlSerializer class which is used to > serializer/deserialize controls before transmitting them on the wire. > > The IPAProxyWorker, which creates and manages the process the IPA runs into, > does not reset its ControlSerializer upon an IPA::configure() call, while > the IPAProxy does so, effectively creating a misalignment between the > two sides of the fence. > > This obviously creates issues as one side of the IPC runs with a > populated and possibly stale cache of ControlInfoMap references, while the > other side gets reset every time a new configuration is applied to the > Camera. > > Fix that by resetting the IPAProxyWorker ControlSerializer on an > IPA configure() call. > > This change fixes an issue which is easily triggered by running two > consecutive capture sessions with the IPA running in isolated mode: > ERROR Serializer control_serializer.cpp:520 Can't deserialize ControlList: unknown ControlInfoMap > > Fixes: 7832e19a599e ("utils: ipc: add templates for code generation for IPC mechanism") > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl > index 8a88bd467da7..22cbc15c5eff 100644 > --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl > @@ -79,6 +79,10 @@ public: > > {% for method in interface_main.methods %} > case {{cmd_enum_name}}::{{method.mojom_name|cap}}: { > + > +{%- if method.mojom_name == "configure" %} > + controlSerializer_.reset(); > +{%- endif %} > {{proxy_funcs.deserialize_call(method|method_param_inputs, '_ipcMessage.data()', '_ipcMessage.fds()', false, true)|indent(8, true)}} > {% for param in method|method_param_outputs %} > {{param|name}} {{param.mojom_name}};
On 03/09/2021 12:12, Laurent Pinchart wrote: > On Fri, Sep 03, 2021 at 06:33:30PM +0900, paul.elder@ideasonboard.com wrote: >> On Thu, Sep 02, 2021 at 02:16:27PM +0100, Kieran Bingham wrote: >>> On 01/09/2021 15:37, Jacopo Mondi wrote: >>>> The IPA::configure() function has an IPAConfigInfo parameters which >>>> contains a map of numerical indexes to ControlInfoMap instances. >>>> >>>> This is a leftover of the old IPA protocol, where it was not possible to >>>> specify a rich interface as it is possible today and each entity >>>> ControlInfoMap was indexed by a numerical id and stored in a map. >>>> >>>> Now that the IPA interface allows to specify parameters by name, drop the >>>> map and send the sensor's control info map only. >>>> >>>> If we'll need more ControlInfoMap to be shared with the IPA, a new parameter >>>> can be added to IPAConfigInfo. >>> >>> >>> It looks like this will need a patch for >>> >>> https://git.libcamera.org/libcamera/ipu3-ipa.git/ >>> >>> as well. But given that it requires manually installed AIQ libraries on >>> non CrOS build environments, perhaps it's something myself or Umang >>> should handle when this series merges. >>> >>> I wonder if we should be increasing the IPA ABI / version numbers too >>> whenever we make a change. >> >> We don't have a release yet so it should be fine, right? :p >> >> I guess we gotta start working on this for real... I'll prepare >> a summary + discussion points. > > Another can of worms :-) It certainly needs to be addressed, but will > need to include .mojom versioning, and protocol stability. We can start > discussions, but I don't think the implementation is the priority at > this point. But we /do/ have externally built IPAs already which can get out of sync on this - and therefore crash. (I know, it's crashed on me already :D) Hiding behind "We don't have a release" only gets us to the point that we release. If that is going to be in 5 years, sure we can keep hiding. But I think we're doing ourselves a dis-service by not actually putting the process in place to use (and therefore test) this before we get to 1.0 >>> But alas - it doesn't look like that's fully implemented, The mojo files >>> should have a version number in there (or even better, could mojo create >>> a hash of the interface to catch when it changes I wonder)? >>> >>> So, given that you can't update the other IPA, and you can't update the >>> IPU3 IPA Version (without further patches) ... I guess this is it ... >>> >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>>> --- >>>> include/libcamera/ipa/ipu3.mojom | 2 +- >>>> src/ipa/ipu3/ipu3.cpp | 4 ++-- >>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- >>>> 3 files changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom >>>> index d561c2244907..2045ce909a88 100644 >>>> --- a/include/libcamera/ipa/ipu3.mojom >>>> +++ b/include/libcamera/ipa/ipu3.mojom >>>> @@ -32,7 +32,7 @@ struct IPU3Action { >>>> >>>> struct IPAConfigInfo { >>>> libcamera.IPACameraSensorInfo sensorInfo; >>>> - map<uint32, libcamera.ControlInfoMap> entityControls; >>>> + libcamera.ControlInfoMap sensorControls; >>>> libcamera.Size bdsOutputSize; >>>> libcamera.Size iif; >>>> }; >>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >>>> index c925cf642611..6a74f7826332 100644 >>>> --- a/src/ipa/ipu3/ipu3.cpp >>>> +++ b/src/ipa/ipu3/ipu3.cpp >>>> @@ -337,14 +337,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize) >>>> >>>> int IPAIPU3::configure(const IPAConfigInfo &configInfo) >>>> { >>>> - if (configInfo.entityControls.empty()) { >>>> + if (configInfo.sensorControls.empty()) { >>>> LOG(IPAIPU3, Error) << "No controls provided"; >>>> return -ENODATA; >>>> } >>>> >>>> sensorInfo_ = configInfo.sensorInfo; >>>> >>>> - ctrls_ = configInfo.entityControls.at(0); >>>> + ctrls_ = configInfo.sensorControls; >>>> >>>> const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE); >>>> if (itExp == ctrls_.end()) { >>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp >>>> index c287bf86e79a..92e869257e53 100644 >>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >>>> @@ -654,7 +654,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) >>>> } >>>> >>>> ipa::ipu3::IPAConfigInfo configInfo; >>>> - configInfo.entityControls.emplace(0, data->cio2_.sensor()->controls()); >>>> + configInfo.sensorControls = data->cio2_.sensor()->controls(); >>>> configInfo.sensorInfo = sensorInfo; >>>> configInfo.bdsOutputSize = config->imguConfig().bds; >>>> configInfo.iif = config->imguConfig().iif; >
Hi Kieran, On Fri, Sep 03, 2021 at 12:30:17PM +0100, Kieran Bingham wrote: > On 03/09/2021 12:12, Laurent Pinchart wrote: > > On Fri, Sep 03, 2021 at 06:33:30PM +0900, paul.elder@ideasonboard.com wrote: > >> On Thu, Sep 02, 2021 at 02:16:27PM +0100, Kieran Bingham wrote: > >>> On 01/09/2021 15:37, Jacopo Mondi wrote: > >>>> The IPA::configure() function has an IPAConfigInfo parameters which > >>>> contains a map of numerical indexes to ControlInfoMap instances. > >>>> > >>>> This is a leftover of the old IPA protocol, where it was not possible to > >>>> specify a rich interface as it is possible today and each entity > >>>> ControlInfoMap was indexed by a numerical id and stored in a map. > >>>> > >>>> Now that the IPA interface allows to specify parameters by name, drop the > >>>> map and send the sensor's control info map only. > >>>> > >>>> If we'll need more ControlInfoMap to be shared with the IPA, a new parameter > >>>> can be added to IPAConfigInfo. > >>> > >>> > >>> It looks like this will need a patch for > >>> > >>> https://git.libcamera.org/libcamera/ipu3-ipa.git/ > >>> > >>> as well. But given that it requires manually installed AIQ libraries on > >>> non CrOS build environments, perhaps it's something myself or Umang > >>> should handle when this series merges. > >>> > >>> I wonder if we should be increasing the IPA ABI / version numbers too > >>> whenever we make a change. > >> > >> We don't have a release yet so it should be fine, right? :p > >> > >> I guess we gotta start working on this for real... I'll prepare > >> a summary + discussion points. > > > > Another can of worms :-) It certainly needs to be addressed, but will > > need to include .mojom versioning, and protocol stability. We can start > > discussions, but I don't think the implementation is the priority at > > this point. > > But we /do/ have externally built IPAs already which can get out of sync > on this - and therefore crash. (I know, it's crashed on me already :D) > > Hiding behind "We don't have a release" only gets us to the point that > we release. If that is going to be in 5 years, sure we can keep hiding. > > But I think we're doing ourselves a dis-service by not actually putting > the process in place to use (and therefore test) this before we get to 1.0 The process has to be put in place, it will involve lots of work, which means that starting the discussion early is good. The implementation, however, isn't the highest priority. > >>> But alas - it doesn't look like that's fully implemented, The mojo files > >>> should have a version number in there (or even better, could mojo create > >>> a hash of the interface to catch when it changes I wonder)? > >>> > >>> So, given that you can't update the other IPA, and you can't update the > >>> IPU3 IPA Version (without further patches) ... I guess this is it ... > >>> > >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>> > >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>>> --- > >>>> include/libcamera/ipa/ipu3.mojom | 2 +- > >>>> src/ipa/ipu3/ipu3.cpp | 4 ++-- > >>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- > >>>> 3 files changed, 4 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom > >>>> index d561c2244907..2045ce909a88 100644 > >>>> --- a/include/libcamera/ipa/ipu3.mojom > >>>> +++ b/include/libcamera/ipa/ipu3.mojom > >>>> @@ -32,7 +32,7 @@ struct IPU3Action { > >>>> > >>>> struct IPAConfigInfo { > >>>> libcamera.IPACameraSensorInfo sensorInfo; > >>>> - map<uint32, libcamera.ControlInfoMap> entityControls; > >>>> + libcamera.ControlInfoMap sensorControls; > >>>> libcamera.Size bdsOutputSize; > >>>> libcamera.Size iif; > >>>> }; > >>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > >>>> index c925cf642611..6a74f7826332 100644 > >>>> --- a/src/ipa/ipu3/ipu3.cpp > >>>> +++ b/src/ipa/ipu3/ipu3.cpp > >>>> @@ -337,14 +337,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize) > >>>> > >>>> int IPAIPU3::configure(const IPAConfigInfo &configInfo) > >>>> { > >>>> - if (configInfo.entityControls.empty()) { > >>>> + if (configInfo.sensorControls.empty()) { > >>>> LOG(IPAIPU3, Error) << "No controls provided"; > >>>> return -ENODATA; > >>>> } > >>>> > >>>> sensorInfo_ = configInfo.sensorInfo; > >>>> > >>>> - ctrls_ = configInfo.entityControls.at(0); > >>>> + ctrls_ = configInfo.sensorControls; > >>>> > >>>> const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE); > >>>> if (itExp == ctrls_.end()) { > >>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > >>>> index c287bf86e79a..92e869257e53 100644 > >>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >>>> @@ -654,7 +654,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > >>>> } > >>>> > >>>> ipa::ipu3::IPAConfigInfo configInfo; > >>>> - configInfo.entityControls.emplace(0, data->cio2_.sensor()->controls()); > >>>> + configInfo.sensorControls = data->cio2_.sensor()->controls(); > >>>> configInfo.sensorInfo = sensorInfo; > >>>> configInfo.bdsOutputSize = config->imguConfig().bds; > >>>> configInfo.iif = config->imguConfig().iif; > >
Hi Jacopo, Thank you for the patch. On Wed, Sep 01, 2021 at 04:37:57PM +0200, Jacopo Mondi wrote: > When running the IPA in isolated mode, each side of the IPC boundary > has an instance of the ControlSerializer class. > > Each serializer instance tags with a numerical id (handle) each > ControlInfoMap instance it serializes, to be capable of associating > ControlList with it at serialization and deserialization time, and > increments the numerical counter for each newly serialized control info > map. > > Having two instances of the ControlSerializer class running in two > different process spaces, each instance increments its own counter, > preventing from maintaining a globally shared state where each > ControlInfoMap instance is reference by a unique identifier. > > Fix this by advancing the serial_ counter at ControlInfoMap > de-serialization time, so that each newly serialized map will have a > globally unique identifier. That's an interesting one. The control serializer was developed with the assumption that a ControlInfoMap would only be sent from the pipeline handler to the IPA, not the other way around. > Fixes: 2c5f0ad23aa4 ("libcamera: Add controls serializer") Technically, the breakage was introduced by the first change in the IPA protocol that sent a ControlInfoMap in the IPA to pipeline handler direction, but I don't mind keeping the Fixes tag as-is. > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/control_serializer.cpp | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > index 27360526f9eb..08cfecca3078 100644 > --- a/src/libcamera/control_serializer.cpp > +++ b/src/libcamera/control_serializer.cpp > @@ -380,6 +380,9 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > return {}; > } > > + /* Keep the info map handles in sync between the two sides of IPC. */ > + serial_ = std::max(serial_, hdr->handle); Can it be this simple ? What if the pipeline handler sends a ControlInfoMap to the IPA at the same time as the IPA sends a ControlInfoMap to the pipeline handler ? Each side will increment their serial number, but the same number will refer to two different maps. > + > /* > * Use the ControlIdMap corresponding to the id map type. If the type > * references a globally defined id map (such as controls::controls
On Fri, Sep 03, 2021 at 02:42:28PM +0900, paul.elder@ideasonboard.com wrote: > On Wed, Sep 01, 2021 at 04:38:00PM +0200, Jacopo Mondi wrote: > > The ControlInfo class was originally designed to only transport > > the control's minimum and maximum values which represent the control's > > valid limits. > > > > Later the default value of the control has been added to the ControlInfo > > class, but the control serializer implementation has not been updated > > accordingly. > > > > This causes issues to IPA modules making use of ControlInfo::def() as, > > s/to/in/ > > > when running in isolation, they would receive a 0 value as default. > > s/ as default// > > There's no way to turn off receiving the "default" :D > > > Fix that by serializing and deserializing the additional ControlValue > > and update the protocol description accordingly. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/control_serializer.cpp | 6 ++++-- > > src/libcamera/ipa_controls.cpp | 14 ++++++++------ > > 2 files changed, 12 insertions(+), 8 deletions(-) > > > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > > index 7eef041a16ee..4e8045251938 100644 > > --- a/src/libcamera/control_serializer.cpp > > +++ b/src/libcamera/control_serializer.cpp > > @@ -105,7 +105,7 @@ size_t ControlSerializer::binarySize(const ControlValue &value) > > > > size_t ControlSerializer::binarySize(const ControlInfo &info) > > { > > - return binarySize(info.min()) + binarySize(info.max()); > > + return binarySize(info.min()) + binarySize(info.max()) + binarySize(info.def()); > > } > > > > /** > > @@ -158,6 +158,7 @@ void ControlSerializer::store(const ControlInfo &info, ByteStreamBuffer &buffer) > > { > > store(info.min(), buffer); > > store(info.max(), buffer); > > + store(info.def(), buffer); > > } > > > > /** > > @@ -346,8 +347,9 @@ ControlInfo ControlSerializer::loadControlInfo(ControlType type, > > > > ControlValue min = loadControlValue(type, b); > > ControlValue max = loadControlValue(type, b); > > + ControlValue def = loadControlValue(type, b); > > > > - return ControlInfo(min, max); > > + return ControlInfo(min, max, def); > > } > > > > /** > > diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp > > index fb98cda30ede..3d172d66b30f 100644 > > --- a/src/libcamera/ipa_controls.cpp > > +++ b/src/libcamera/ipa_controls.cpp > > @@ -108,17 +108,19 @@ > > * +-------------------------+ . > > * / | ... | | entry[n].offset > > * | +-------------------------+ <-----´ > > - * Data | | minimum value (#n) | \ > > - * section | +-------------------------+ | Entry #n > > - * | | maximum value (#n) | / > > + * | | minimum value (#n) | \ > > + * Data | +-------------------------+ | > > + * section | | maximum value (#n) | | Entry #n > > + * | +-------------------------+ | > > + * | | default value (#n) | / > > * | +-------------------------+ > > * \ | ... | > > * +-------------------------+ > > * ~~~~ > > * > > - * The minimum and maximum value are stored in the platform's native data > > - * format. The ipa_control_info_entry::offset field stores the offset from the > > - * beginning of the data section to the info data. > > + * The minimum, maximum and default value are stored in the platform's native > > s/maximum/maximum,/ Oxford commas https://en.wikipedia.org/wiki/Serial_comma :-) (I would have gone without it here, or have kept value as a singular to only refer to default.) > s/value/values/ Agreed. > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > + * data format. The ipa_control_info_entry::offset field stores the offset from > > + * the beginning of the data section to the info data. > > * > > * Info data in the data section shall be stored in the same order as the > > * entries array, shall be aligned to a multiple of 8 bytes, and shall be
Hi Jacopo, Thank you for the patch. On Fri, Sep 03, 2021 at 11:21:46AM +0200, Jacopo Mondi wrote: > On Thu, Sep 02, 2021 at 03:34:01PM +0100, Kieran Bingham wrote: > > On 01/09/2021 15:37, Jacopo Mondi wrote: > > > When a ControlList is deserialized, the code searches for a valid > > > ControlInfoMap in the local cache and use its id map to initialize the > > > list. If no valid ControlInfoMap is found, as it's usually the case > > > for lists transporting libcamera controls and properties, the globally > > > defined controls::controls id map is used unconditionally. > > > > > > This breaks the deserialization of libcamera properties, for which a > > > wrong idmap is used at construction time. When does this happen ? Shouldn't we ensure that we first serialize a ControInfoMap for the properties ? Or do we not have that ? I'm OK with this patch as a short-term fix, but I'd like to understand the big picture. > > > As the serialization header now transports an id_map_type field, store > > > the idmap type at serialization time, and re-use it at > > > deserialization time to identify the correct id map. > > > > > > Also make the validation stricter by imposing to list of V4L2 controls to > > > have an associated ControlInfoMap available, as there is no globally > > > defined idmap for such controls. > > > > > > To be able to retrieve the idmap associated with a ControlList, add an > > > accessor function to the ControlList class. > > > > > > It might be worth in future using a ControlInfoMap to initialize the > > > deserialized ControlList to implement controls validation against their > > > limit. As such validation is not implemented at the moment, maintain the > > > current behaviour and initialize the control list with an idmap. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > include/libcamera/controls.h | 1 + > > > src/libcamera/control_serializer.cpp | 49 +++++++++++++++++++++++----- > > > src/libcamera/controls.cpp | 8 +++++ > > > 3 files changed, 49 insertions(+), 9 deletions(-) > > > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > > index 6668e4bb1053..8e5bc8b70b94 100644 > > > --- a/include/libcamera/controls.h > > > +++ b/include/libcamera/controls.h > > > @@ -407,6 +407,7 @@ public: > > > void set(unsigned int id, const ControlValue &value); > > > > > > const ControlInfoMap *infoMap() const { return infoMap_; } > > > + const ControlIdMap *idMap() const { return idmap_; } > > > > > > private: > > > const ControlValue *find(unsigned int id) const; > > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > > > index 08cfecca3078..d4377c024079 100644 > > > --- a/src/libcamera/control_serializer.cpp > > > +++ b/src/libcamera/control_serializer.cpp > > > @@ -275,6 +275,15 @@ int ControlSerializer::serialize(const ControlList &list, > > > infoMapHandle = 0; > > > } > > > > > > + const ControlIdMap *idmap = list.idMap(); > > > + enum ipa_controls_id_map_type idMapType; > > > + if (idmap == &controls::controls) > > > + idMapType = IPA_CONTROL_ID_MAP_CONTROLS; > > > + else if (idmap == &properties::properties) > > > + idMapType = IPA_CONTROL_ID_MAP_PROPERTIES; > > > + else > > > + idMapType = IPA_CONTROL_ID_MAP_V4L2; > > > + > > > size_t entriesSize = list.size() * sizeof(struct ipa_control_value_entry); > > > size_t valuesSize = 0; > > > for (const auto &ctrl : list) > > > @@ -287,6 +296,7 @@ int ControlSerializer::serialize(const ControlList &list, > > > hdr.entries = list.size(); > > > hdr.size = sizeof(hdr) + entriesSize + valuesSize; > > > hdr.data_offset = sizeof(hdr) + entriesSize; > > > + hdr.id_map_type = idMapType; > > > > > > buffer.write(&hdr); > > > > > > @@ -499,13 +509,15 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > > > } > > > > > > /* > > > - * Retrieve the ControlInfoMap associated with the ControlList based on > > > - * its ID. The mapping between infoMap and ID is set up when serializing > > > - * or deserializing ControlInfoMap. If no mapping is found (which is > > > - * currently the case for ControlList related to libcamera controls), > > > - * use the global control::control idmap. > > > + * Retrieve the ControlIdMap associated with the ControlList. > > > + * > > > + * The idmap is either retrieved from the list's ControlInfoMap when > > > + * a valid handle has been initialized at serialization time, or by > > > + * using the header's id_map_type field for lists that refer to the > > > + * globally defined libcamera controls and properties, for which no > > > + * ControlInfoMap is available. > > > */ > > > - const ControlInfoMap *infoMap; > > > + const ControlIdMap *idMap; > > > if (hdr->handle) { > > > auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(), > > > [&](decltype(infoMapHandles_)::value_type &entry) { > > > @@ -517,12 +529,31 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > > > return {}; > > > } > > > > > > - infoMap = iter->first; > > > + const ControlInfoMap *infoMap = iter->first; > > > + idMap = &infoMap->idmap(); > > > } else { > > > - infoMap = nullptr; > > > + switch (hdr->id_map_type) { > > > + case IPA_CONTROL_ID_MAP_CONTROLS: > > > + idMap = &controls::controls; > > > + break; > > > + > > > + case IPA_CONTROL_ID_MAP_PROPERTIES: > > > + idMap = &properties::properties; > > > + break; > > Ups, missing a line break here! > > > > + case IPA_CONTROL_ID_MAP_V4L2: > > > + LOG(Serializer, Fatal) > > > + << "A list of V4L2 controls requires an ControlInfoMap"; > > > > Can we have a return statement after the Fatal? > > > > (Presuming that there is no safe path forwards if Fatal is lowered to > > Error on Release builds). > > Right! In production builds this won't exit. I'll add a return > > > > + } > > > } > > > > > > - ControlList ctrls(infoMap ? infoMap->idmap() : controls::controls); > > > + /* > > > + * \todo If possible, initialize the list with the ControlInfoMap > > > + * so that controls can be validated against their limits. > > > + * Currently no validation is performed, so it's fine relying on the > > > + * idmap only. > > > + */ > > > + ASSERT(idMap); > > > > I presume this assert can only be hit through the > > IPA_CONTROL_ID_MAP_V4L2 path above? > > It could actually be hit through the if (hdr->handle) branch > where the idMap is retrieved from the infoMap Can this happen in practice, or would it be a bug somewhere ? > > Although actually - if there's ever a new IPA_CONTROL_ID_MAP type added, > > it guards against that too - so the assert is good to have. > > If we add a new IPA_CONTROL_ID_MAP type the compiler will warn that > not all the cases are handled in the switch statment, as I didn't add > a default: case precisely for that reason. > > So yes, I could move the ASSERT() in the if() {} branch actually. Sounds good to me. > > > > Only minors there: > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > + ControlList ctrls(*idMap); > > > > > > for (unsigned int i = 0; i < hdr->entries; ++i) { > > > const struct ipa_control_value_entry *entry = > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > > index 5c05ba4a7cd0..96625edb1380 100644 > > > --- a/src/libcamera/controls.cpp > > > +++ b/src/libcamera/controls.cpp > > > @@ -1038,6 +1038,14 @@ void ControlList::set(unsigned int id, const ControlValue &value) > > > * associated ControlInfoMap, nullptr is returned in that case. > > > */ > > > > > > +/** > > > + * \fn ControlList::idMap() > > > + * \brief Retrieve the ControlId map used to construct the ControlList > > > + * \return The ControlId map used to construct the ControlList. ControlsList > > > + * instances constructed with ControlList() have no associated idmap, nullptr is > > > + * returned in that case. > > > + */ > > > + > > > const ControlValue *ControlList::find(unsigned int id) const > > > { > > > const auto iter = controls_.find(id);
Hi Laurent, On Fri, Sep 03, 2021 at 02:46:35PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Wed, Sep 01, 2021 at 04:37:57PM +0200, Jacopo Mondi wrote: > > When running the IPA in isolated mode, each side of the IPC boundary > > has an instance of the ControlSerializer class. > > > > Each serializer instance tags with a numerical id (handle) each > > ControlInfoMap instance it serializes, to be capable of associating > > ControlList with it at serialization and deserialization time, and > > increments the numerical counter for each newly serialized control info > > map. > > > > Having two instances of the ControlSerializer class running in two > > different process spaces, each instance increments its own counter, > > preventing from maintaining a globally shared state where each > > ControlInfoMap instance is reference by a unique identifier. > > > > Fix this by advancing the serial_ counter at ControlInfoMap > > de-serialization time, so that each newly serialized map will have a > > globally unique identifier. > > That's an interesting one. The control serializer was developed with the > assumption that a ControlInfoMap would only be sent from the pipeline > handler to the IPA, not the other way around. > > > Fixes: 2c5f0ad23aa4 ("libcamera: Add controls serializer") > > Technically, the breakage was introduced by the first change in the IPA > protocol that sent a ControlInfoMap in the IPA to pipeline handler > direction, but I don't mind keeping the Fixes tag as-is. > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/control_serializer.cpp | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > > index 27360526f9eb..08cfecca3078 100644 > > --- a/src/libcamera/control_serializer.cpp > > +++ b/src/libcamera/control_serializer.cpp > > @@ -380,6 +380,9 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > > return {}; > > } > > > > + /* Keep the info map handles in sync between the two sides of IPC. */ > > + serial_ = std::max(serial_, hdr->handle); > > Can it be this simple ? What if the pipeline handler sends a > ControlInfoMap to the IPA at the same time as the IPA sends a > ControlInfoMap to the pipeline handler ? Each side will increment their > serial number, but the same number will refer to two different maps. > Yes, this might happen during a capture session, where the PH/IPA interactions are aysnchronous. It can't happen before, as there won't be concurrect calls between the two sides of the IPC A regular run looks like ----------------------------------------------------------------------------- PH handle# IPC IPA handl# ----------------------------------------------------------------------------- ser(map) 1 ---> deser(map) max(0,1)=1 <--- ser(map) 2 max(1,2)=2 deser(map) ser(map) 3 ... ----------------------------------------------------------------------------- This could break only if a ser/deser sequence is interleaved with another serialization event ----------------------------------------------------------------------------- PH handle# IPC IPA handl# ----------------------------------------------------------------------------- ser(map) 1 ----> <---- ser(map) 1 deser(map) max(1,1)=1 max(1,1)=1 deser(map) ser(map) 2 ... ----------------------------------------------------------------------------- Which is very unfortunate but possible. We have no centralized state between the two processes and we have no way to assign handles in the proxies, as they have the same information as the control serializer has. This might seem rather stupid, but we have two sides of the IPC, can't we use even handles on one side and odd ones on the other ? The proxies initialize the controls serializer with a seed (0 or 1) and we simply serial_ += 2 ? > > + > > /* > > * Use the ControlIdMap corresponding to the id map type. If the type > > * references a globally defined id map (such as controls::controls > > -- > Regards, > > Laurent Pinchart
Hi Laurent, On Fri, Sep 03, 2021 at 03:58:30PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Fri, Sep 03, 2021 at 11:21:46AM +0200, Jacopo Mondi wrote: > > On Thu, Sep 02, 2021 at 03:34:01PM +0100, Kieran Bingham wrote: > > > On 01/09/2021 15:37, Jacopo Mondi wrote: > > > > When a ControlList is deserialized, the code searches for a valid > > > > ControlInfoMap in the local cache and use its id map to initialize the > > > > list. If no valid ControlInfoMap is found, as it's usually the case > > > > for lists transporting libcamera controls and properties, the globally > > > > defined controls::controls id map is used unconditionally. > > > > > > > > This breaks the deserialization of libcamera properties, for which a > > > > wrong idmap is used at construction time. > > When does this happen ? Shouldn't we ensure that we first serialize a > ControInfoMap for the properties ? Or do we not have that ? I'm OK with > this patch as a short-term fix, but I'd like to understand the big > picture. > Properties are intialized using the properties::properties idmap, as having a ControlInfoMap for unmutable controls like properties are doesn't make much sense. Hence they won't even have an handle associated, and the current implementation uses controls::controls as idmap if not handle is available. We don't triggere the issue as we don't serialize properties, but as soon as it happens, we're screwed. And yes, we can catch it during review but we'll forget or we carry around the issue untile someone gets fed up and addresses it (see ControlInfoMap::def() which is a small change that was due since a year or so). Given that the patch is not super complex I would rather fix it now :) > > > > As the serialization header now transports an id_map_type field, store > > > > the idmap type at serialization time, and re-use it at > > > > deserialization time to identify the correct id map. > > > > > > > > Also make the validation stricter by imposing to list of V4L2 controls to > > > > have an associated ControlInfoMap available, as there is no globally > > > > defined idmap for such controls. > > > > > > > > To be able to retrieve the idmap associated with a ControlList, add an > > > > accessor function to the ControlList class. > > > > > > > > It might be worth in future using a ControlInfoMap to initialize the > > > > deserialized ControlList to implement controls validation against their > > > > limit. As such validation is not implemented at the moment, maintain the > > > > current behaviour and initialize the control list with an idmap. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > include/libcamera/controls.h | 1 + > > > > src/libcamera/control_serializer.cpp | 49 +++++++++++++++++++++++----- > > > > src/libcamera/controls.cpp | 8 +++++ > > > > 3 files changed, 49 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > > > index 6668e4bb1053..8e5bc8b70b94 100644 > > > > --- a/include/libcamera/controls.h > > > > +++ b/include/libcamera/controls.h > > > > @@ -407,6 +407,7 @@ public: > > > > void set(unsigned int id, const ControlValue &value); > > > > > > > > const ControlInfoMap *infoMap() const { return infoMap_; } > > > > + const ControlIdMap *idMap() const { return idmap_; } > > > > > > > > private: > > > > const ControlValue *find(unsigned int id) const; > > > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > > > > index 08cfecca3078..d4377c024079 100644 > > > > --- a/src/libcamera/control_serializer.cpp > > > > +++ b/src/libcamera/control_serializer.cpp > > > > @@ -275,6 +275,15 @@ int ControlSerializer::serialize(const ControlList &list, > > > > infoMapHandle = 0; > > > > } > > > > > > > > + const ControlIdMap *idmap = list.idMap(); > > > > + enum ipa_controls_id_map_type idMapType; > > > > + if (idmap == &controls::controls) > > > > + idMapType = IPA_CONTROL_ID_MAP_CONTROLS; > > > > + else if (idmap == &properties::properties) > > > > + idMapType = IPA_CONTROL_ID_MAP_PROPERTIES; > > > > + else > > > > + idMapType = IPA_CONTROL_ID_MAP_V4L2; > > > > + > > > > size_t entriesSize = list.size() * sizeof(struct ipa_control_value_entry); > > > > size_t valuesSize = 0; > > > > for (const auto &ctrl : list) > > > > @@ -287,6 +296,7 @@ int ControlSerializer::serialize(const ControlList &list, > > > > hdr.entries = list.size(); > > > > hdr.size = sizeof(hdr) + entriesSize + valuesSize; > > > > hdr.data_offset = sizeof(hdr) + entriesSize; > > > > + hdr.id_map_type = idMapType; > > > > > > > > buffer.write(&hdr); > > > > > > > > @@ -499,13 +509,15 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > > > > } > > > > > > > > /* > > > > - * Retrieve the ControlInfoMap associated with the ControlList based on > > > > - * its ID. The mapping between infoMap and ID is set up when serializing > > > > - * or deserializing ControlInfoMap. If no mapping is found (which is > > > > - * currently the case for ControlList related to libcamera controls), > > > > - * use the global control::control idmap. > > > > + * Retrieve the ControlIdMap associated with the ControlList. > > > > + * > > > > + * The idmap is either retrieved from the list's ControlInfoMap when > > > > + * a valid handle has been initialized at serialization time, or by > > > > + * using the header's id_map_type field for lists that refer to the > > > > + * globally defined libcamera controls and properties, for which no > > > > + * ControlInfoMap is available. > > > > */ > > > > - const ControlInfoMap *infoMap; > > > > + const ControlIdMap *idMap; > > > > if (hdr->handle) { > > > > auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(), > > > > [&](decltype(infoMapHandles_)::value_type &entry) { > > > > @@ -517,12 +529,31 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > > > > return {}; > > > > } > > > > > > > > - infoMap = iter->first; > > > > + const ControlInfoMap *infoMap = iter->first; > > > > + idMap = &infoMap->idmap(); > > > > } else { > > > > - infoMap = nullptr; > > > > + switch (hdr->id_map_type) { > > > > + case IPA_CONTROL_ID_MAP_CONTROLS: > > > > + idMap = &controls::controls; > > > > + break; > > > > + > > > > + case IPA_CONTROL_ID_MAP_PROPERTIES: > > > > + idMap = &properties::properties; > > > > + break; > > > > Ups, missing a line break here! > > > > > > + case IPA_CONTROL_ID_MAP_V4L2: > > > > + LOG(Serializer, Fatal) > > > > + << "A list of V4L2 controls requires an ControlInfoMap"; > > > > > > Can we have a return statement after the Fatal? > > > > > > (Presuming that there is no safe path forwards if Fatal is lowered to > > > Error on Release builds). > > > > Right! In production builds this won't exit. I'll add a return > > > > > > + } > > > > } > > > > > > > > - ControlList ctrls(infoMap ? infoMap->idmap() : controls::controls); > > > > + /* > > > > + * \todo If possible, initialize the list with the ControlInfoMap > > > > + * so that controls can be validated against their limits. > > > > + * Currently no validation is performed, so it's fine relying on the > > > > + * idmap only. > > > > + */ > > > > + ASSERT(idMap); > > > > > > I presume this assert can only be hit through the > > > IPA_CONTROL_ID_MAP_V4L2 path above? > > > > It could actually be hit through the if (hdr->handle) branch > > where the idMap is retrieved from the infoMap > > Can this happen in practice, or would it be a bug somewhere ? Only if someone tries to serialize a ControlList constructed through the default constructor, which is unlikely, but possible. > > > > Although actually - if there's ever a new IPA_CONTROL_ID_MAP type added, > > > it guards against that too - so the assert is good to have. > > > > If we add a new IPA_CONTROL_ID_MAP type the compiler will warn that > > not all the cases are handled in the switch statment, as I didn't add > > a default: case precisely for that reason. > > > > So yes, I could move the ASSERT() in the if() {} branch actually. > > Sounds good to me. > Thanks j > > > > > > Only minors there: > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > + ControlList ctrls(*idMap); > > > > > > > > for (unsigned int i = 0; i < hdr->entries; ++i) { > > > > const struct ipa_control_value_entry *entry = > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > > > index 5c05ba4a7cd0..96625edb1380 100644 > > > > --- a/src/libcamera/controls.cpp > > > > +++ b/src/libcamera/controls.cpp > > > > @@ -1038,6 +1038,14 @@ void ControlList::set(unsigned int id, const ControlValue &value) > > > > * associated ControlInfoMap, nullptr is returned in that case. > > > > */ > > > > > > > > +/** > > > > + * \fn ControlList::idMap() > > > > + * \brief Retrieve the ControlId map used to construct the ControlList > > > > + * \return The ControlId map used to construct the ControlList. ControlsList > > > > + * instances constructed with ControlList() have no associated idmap, nullptr is > > > > + * returned in that case. > > > > + */ > > > > + > > > > const ControlValue *ControlList::find(unsigned int id) const > > > > { > > > > const auto iter = controls_.find(id); > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Fri, Sep 03, 2021 at 03:36:15PM +0200, Jacopo Mondi wrote: > On Fri, Sep 03, 2021 at 03:58:30PM +0300, Laurent Pinchart wrote: > > On Fri, Sep 03, 2021 at 11:21:46AM +0200, Jacopo Mondi wrote: > > > On Thu, Sep 02, 2021 at 03:34:01PM +0100, Kieran Bingham wrote: > > > > On 01/09/2021 15:37, Jacopo Mondi wrote: > > > > > When a ControlList is deserialized, the code searches for a valid > > > > > ControlInfoMap in the local cache and use its id map to initialize the > > > > > list. If no valid ControlInfoMap is found, as it's usually the case > > > > > for lists transporting libcamera controls and properties, the globally > > > > > defined controls::controls id map is used unconditionally. > > > > > > > > > > This breaks the deserialization of libcamera properties, for which a > > > > > wrong idmap is used at construction time. > > > > When does this happen ? Shouldn't we ensure that we first serialize a > > ControInfoMap for the properties ? Or do we not have that ? I'm OK with > > this patch as a short-term fix, but I'd like to understand the big > > picture. > > Properties are intialized using the properties::properties idmap, as > having a ControlInfoMap for unmutable controls like properties are > doesn't make much sense. Hence they won't even have an handle > associated, and the current implementation uses controls::controls as > idmap if not handle is available. > > We don't triggere the issue as we don't serialize properties, but as > soon as it happens, we're screwed. > > And yes, we can catch it during review but we'll forget or we carry > around the issue untile someone gets fed up and addresses it (see > ControlInfoMap::def() which is a small change that was due since a > year or so). > > Given that the patch is not super complex I would rather fix it now :) Fine with me. I think sending properties from the IPA to the pipeline handler at init time is probably a valid use case. > > > > > As the serialization header now transports an id_map_type field, store > > > > > the idmap type at serialization time, and re-use it at > > > > > deserialization time to identify the correct id map. > > > > > > > > > > Also make the validation stricter by imposing to list of V4L2 controls to > > > > > have an associated ControlInfoMap available, as there is no globally > > > > > defined idmap for such controls. > > > > > > > > > > To be able to retrieve the idmap associated with a ControlList, add an > > > > > accessor function to the ControlList class. > > > > > > > > > > It might be worth in future using a ControlInfoMap to initialize the > > > > > deserialized ControlList to implement controls validation against their > > > > > limit. As such validation is not implemented at the moment, maintain the > > > > > current behaviour and initialize the control list with an idmap. > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > --- > > > > > include/libcamera/controls.h | 1 + > > > > > src/libcamera/control_serializer.cpp | 49 +++++++++++++++++++++++----- > > > > > src/libcamera/controls.cpp | 8 +++++ > > > > > 3 files changed, 49 insertions(+), 9 deletions(-) > > > > > > > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > > > > index 6668e4bb1053..8e5bc8b70b94 100644 > > > > > --- a/include/libcamera/controls.h > > > > > +++ b/include/libcamera/controls.h > > > > > @@ -407,6 +407,7 @@ public: > > > > > void set(unsigned int id, const ControlValue &value); > > > > > > > > > > const ControlInfoMap *infoMap() const { return infoMap_; } > > > > > + const ControlIdMap *idMap() const { return idmap_; } > > > > > > > > > > private: > > > > > const ControlValue *find(unsigned int id) const; > > > > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > > > > > index 08cfecca3078..d4377c024079 100644 > > > > > --- a/src/libcamera/control_serializer.cpp > > > > > +++ b/src/libcamera/control_serializer.cpp > > > > > @@ -275,6 +275,15 @@ int ControlSerializer::serialize(const ControlList &list, > > > > > infoMapHandle = 0; > > > > > } > > > > > > > > > > + const ControlIdMap *idmap = list.idMap(); > > > > > + enum ipa_controls_id_map_type idMapType; > > > > > + if (idmap == &controls::controls) > > > > > + idMapType = IPA_CONTROL_ID_MAP_CONTROLS; > > > > > + else if (idmap == &properties::properties) > > > > > + idMapType = IPA_CONTROL_ID_MAP_PROPERTIES; > > > > > + else > > > > > + idMapType = IPA_CONTROL_ID_MAP_V4L2; > > > > > + > > > > > size_t entriesSize = list.size() * sizeof(struct ipa_control_value_entry); > > > > > size_t valuesSize = 0; > > > > > for (const auto &ctrl : list) > > > > > @@ -287,6 +296,7 @@ int ControlSerializer::serialize(const ControlList &list, > > > > > hdr.entries = list.size(); > > > > > hdr.size = sizeof(hdr) + entriesSize + valuesSize; > > > > > hdr.data_offset = sizeof(hdr) + entriesSize; > > > > > + hdr.id_map_type = idMapType; > > > > > > > > > > buffer.write(&hdr); > > > > > > > > > > @@ -499,13 +509,15 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > > > > > } > > > > > > > > > > /* > > > > > - * Retrieve the ControlInfoMap associated with the ControlList based on > > > > > - * its ID. The mapping between infoMap and ID is set up when serializing > > > > > - * or deserializing ControlInfoMap. If no mapping is found (which is > > > > > - * currently the case for ControlList related to libcamera controls), > > > > > - * use the global control::control idmap. > > > > > + * Retrieve the ControlIdMap associated with the ControlList. > > > > > + * > > > > > + * The idmap is either retrieved from the list's ControlInfoMap when > > > > > + * a valid handle has been initialized at serialization time, or by > > > > > + * using the header's id_map_type field for lists that refer to the > > > > > + * globally defined libcamera controls and properties, for which no > > > > > + * ControlInfoMap is available. > > > > > */ > > > > > - const ControlInfoMap *infoMap; > > > > > + const ControlIdMap *idMap; > > > > > if (hdr->handle) { > > > > > auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(), > > > > > [&](decltype(infoMapHandles_)::value_type &entry) { > > > > > @@ -517,12 +529,31 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > > > > > return {}; > > > > > } > > > > > > > > > > - infoMap = iter->first; > > > > > + const ControlInfoMap *infoMap = iter->first; > > > > > + idMap = &infoMap->idmap(); > > > > > } else { > > > > > - infoMap = nullptr; > > > > > + switch (hdr->id_map_type) { > > > > > + case IPA_CONTROL_ID_MAP_CONTROLS: > > > > > + idMap = &controls::controls; > > > > > + break; > > > > > + > > > > > + case IPA_CONTROL_ID_MAP_PROPERTIES: > > > > > + idMap = &properties::properties; > > > > > + break; > > > > > > Ups, missing a line break here! > > > > > > > > + case IPA_CONTROL_ID_MAP_V4L2: > > > > > + LOG(Serializer, Fatal) > > > > > + << "A list of V4L2 controls requires an ControlInfoMap"; > > > > > > > > Can we have a return statement after the Fatal? > > > > > > > > (Presuming that there is no safe path forwards if Fatal is lowered to > > > > Error on Release builds). > > > > > > Right! In production builds this won't exit. I'll add a return > > > > > > > > + } > > > > > } > > > > > > > > > > - ControlList ctrls(infoMap ? infoMap->idmap() : controls::controls); > > > > > + /* > > > > > + * \todo If possible, initialize the list with the ControlInfoMap > > > > > + * so that controls can be validated against their limits. > > > > > + * Currently no validation is performed, so it's fine relying on the > > > > > + * idmap only. > > > > > + */ > > > > > + ASSERT(idMap); > > > > > > > > I presume this assert can only be hit through the > > > > IPA_CONTROL_ID_MAP_V4L2 path above? > > > > > > It could actually be hit through the if (hdr->handle) branch > > > where the idMap is retrieved from the infoMap > > > > Can this happen in practice, or would it be a bug somewhere ? > > Only if someone tries to serialize a ControlList constructed through > the default constructor, which is unlikely, but possible. I'll consider that as a bug, so ASSERT() is good enough. > > > > Although actually - if there's ever a new IPA_CONTROL_ID_MAP type added, > > > > it guards against that too - so the assert is good to have. > > > > > > If we add a new IPA_CONTROL_ID_MAP type the compiler will warn that > > > not all the cases are handled in the switch statment, as I didn't add > > > a default: case precisely for that reason. > > > > > > So yes, I could move the ASSERT() in the if() {} branch actually. > > > > Sounds good to me. > > Thanks > > > > > Only minors there: > > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > + ControlList ctrls(*idMap); > > > > > > > > > > for (unsigned int i = 0; i < hdr->entries; ++i) { > > > > > const struct ipa_control_value_entry *entry = > > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > > > > index 5c05ba4a7cd0..96625edb1380 100644 > > > > > --- a/src/libcamera/controls.cpp > > > > > +++ b/src/libcamera/controls.cpp > > > > > @@ -1038,6 +1038,14 @@ void ControlList::set(unsigned int id, const ControlValue &value) > > > > > * associated ControlInfoMap, nullptr is returned in that case. > > > > > */ > > > > > > > > > > +/** > > > > > + * \fn ControlList::idMap() > > > > > + * \brief Retrieve the ControlId map used to construct the ControlList > > > > > + * \return The ControlId map used to construct the ControlList. ControlsList > > > > > + * instances constructed with ControlList() have no associated idmap, nullptr is > > > > > + * returned in that case. > > > > > + */ > > > > > + > > > > > const ControlValue *ControlList::find(unsigned int id) const > > > > > { > > > > > const auto iter = controls_.find(id);
On 03/09/2021 10:16, Jacopo Mondi wrote: > Hi Kieran, > > On Thu, Sep 02, 2021 at 03:05:23PM +0100, Kieran Bingham wrote: >> On 01/09/2021 15:37, Jacopo Mondi wrote: >>> When running the IPA in isolated mode, each side of the IPC boundary >>> has an instance of the ControlSerializer class which is used to >>> serializer/deserialize controls before transmitting them on the wire. >>> >>> The IPAProxyWorker, which creates and manages the process the IPA runs into, >>> does not reset its ControlSerializer upon an IPA::configure() call, while >>> the IPAProxy does so, effectively creating a misalignment between the >>> two sides of the fence. >>> >>> This obviously creates issues as one side of the IPC runs with a >>> populated and possibly stale cache of ControlInfoMap references, while the >>> other side gets reset every time a new configuration is applied to the >>> Camera. >>> >>> Fix that by resetting the IPAProxyWorker ControlSerializer on an >>> IPA configure() call. >>> >>> This change fixes an issue which is easily triggered by running two >>> consecutive capture sessions with the IPA running in isolated mode: >>> ERROR Serializer control_serializer.cpp:520 Can't deserialize ControlList: unknown ControlInfoMap >>> >>> Fixes: 7832e19a599e ("utils: ipc: add templates for code generation for IPC mechanism") >> >> Is it also technically fixing the patch that added the reset on the >> other side? as that's when the misalignment begins? >> > > The 'other side' was reset from the very beginning. > > To be honest I would question the usage of Fixes in libcamera, as we > don't have versions to backport to, but I added this mosotly for cargo > cult. > > Iff we want to keep using Fixes I think that's the opportune commit to > point to, as this patch should be ideally fixed-up in that one (if we > could travel back in time) I don't think we have any requirement to add a Fixes tag, except it helps promote awareness of /why/ or /what/ is being fixed. It also helps provide context in some occasions in case the fix turns out to be wrong, so it's partly documentation. Imagine reading back through the history trying to find out why something changed that doesn't seem to make sense. Seeing a Fixes tag helps you see 'Oh, ok - so this change was to fix that patch, so between there and here, the thing isn't going to be right'. And that it's likely a good habit to keep from Kernel side? But no, I don't think we're backporting to a non-existent version. >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>> --- >>> .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl >>> index 8a88bd467da7..22cbc15c5eff 100644 >>> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl >>> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl >>> @@ -79,6 +79,10 @@ public: >>> >>> {% for method in interface_main.methods %} >>> case {{cmd_enum_name}}::{{method.mojom_name|cap}}: { >>> + >>> +{%- if method.mojom_name == "configure" %} >>> + controlSerializer_.reset(); >> >> one more level indentation >> ? >> >> It looks like we're "inside" this case statement, so it should be one >> level deeper. >> > > Looking at the generated code > > case _IPU3Cmd::Configure: { > controlSerializer_.reset(); > > > const size_t configInfoStart = 0; > > > IPAConfigInfo configInfo = IPADataSerializer<IPAConfigInfo>::deserialize( > > You are correct the indentation is wrong, but it does anyway matches > the rest of the file :) Indeed, it matches the next two lines (A,B) - but the rest of the case statement is otherwise correct. If I could figure out how to correct those two lines I would - but I rabbit-holed too long in this template yesterday. I think it's still reasonable to put the controlSerializer_.reset(); at the correct indentation. The args are incorrectly indented, but would be solved by fixing that (waves hand magically) "somewhere else" ;-) : > case _IPU3Cmd::Configure: { > > > A const size_t configInfoStart = 0; > > > B IPAConfigInfo configInfo = IPADataSerializer<IPAConfigInfo>::deserialize( > B _ipcMessage.data().cbegin() + configInfoStart, > B _ipcMessage.data().cend(), > B &controlSerializer_); > > > int32_t _callRet =ipa_->configure(configInfo); > > IPCMessage::Header header = { _ipcMessage.header().cmd, _ipcMessage.header().cookie }; > IPCMessage _response(header); > std::vector<uint8_t> _callRetBuf; > std::tie(_callRetBuf, std::ignore) = > IPADataSerializer<int32_t>::serialize(_callRet); > _response.data().insert(_response.data().end(), _callRetBuf.cbegin(), _callRetBuf.cend()); > > int _ret = socket_.send(_response.payload()); > if (_ret < 0) { > LOG(IPAProxyIPU3Worker, Error) > << "Reply to configure() failed: " << _ret; > } > LOG(IPAProxyIPU3Worker, Debug) << "Done replying to configure()"; > break; > } Paul, Are A and B easy fixes? They appear to be automagically generated to do with the IPADataSerializer for every call? >>> +{%- endif %} >> >> >> Wow, these templates are hard to parse :-) >> >> But ... it looks like this is right ;-) >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >>> {{proxy_funcs.deserialize_call(method|method_param_inputs, '_ipcMessage.data()', '_ipcMessage.fds()', false, true)|indent(8, true)}} >>> {% for param in method|method_param_outputs %} >>> {{param|name}} {{param.mojom_name}}; >>>
Hi Jacopo, On Fri, Sep 03, 2021 at 03:28:41PM +0200, Jacopo Mondi wrote: > On Fri, Sep 03, 2021 at 02:46:35PM +0300, Laurent Pinchart wrote: > > On Wed, Sep 01, 2021 at 04:37:57PM +0200, Jacopo Mondi wrote: > > > When running the IPA in isolated mode, each side of the IPC boundary > > > has an instance of the ControlSerializer class. > > > > > > Each serializer instance tags with a numerical id (handle) each > > > ControlInfoMap instance it serializes, to be capable of associating > > > ControlList with it at serialization and deserialization time, and > > > increments the numerical counter for each newly serialized control info > > > map. > > > > > > Having two instances of the ControlSerializer class running in two > > > different process spaces, each instance increments its own counter, > > > preventing from maintaining a globally shared state where each > > > ControlInfoMap instance is reference by a unique identifier. > > > > > > Fix this by advancing the serial_ counter at ControlInfoMap > > > de-serialization time, so that each newly serialized map will have a > > > globally unique identifier. > > > > That's an interesting one. The control serializer was developed with the > > assumption that a ControlInfoMap would only be sent from the pipeline > > handler to the IPA, not the other way around. > > > > > Fixes: 2c5f0ad23aa4 ("libcamera: Add controls serializer") > > > > Technically, the breakage was introduced by the first change in the IPA > > protocol that sent a ControlInfoMap in the IPA to pipeline handler > > direction, but I don't mind keeping the Fixes tag as-is. > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/control_serializer.cpp | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > > > index 27360526f9eb..08cfecca3078 100644 > > > --- a/src/libcamera/control_serializer.cpp > > > +++ b/src/libcamera/control_serializer.cpp > > > @@ -380,6 +380,9 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > > > return {}; > > > } > > > > > > + /* Keep the info map handles in sync between the two sides of IPC. */ > > > + serial_ = std::max(serial_, hdr->handle); > > > > Can it be this simple ? What if the pipeline handler sends a > > ControlInfoMap to the IPA at the same time as the IPA sends a > > ControlInfoMap to the pipeline handler ? Each side will increment their > > serial number, but the same number will refer to two different maps. > > Yes, this might happen during a capture session, where the PH/IPA > interactions are aysnchronous. It can't happen before, as there won't > be concurrect calls between the two sides of the IPC > > A regular run looks like > > ----------------------------------------------------------------------------- > PH handle# IPC IPA handl# > ----------------------------------------------------------------------------- > ser(map) 1 ---> > deser(map) > max(0,1)=1 > > <--- ser(map) 2 > max(1,2)=2 > deser(map) > > ser(map) 3 > ... > ----------------------------------------------------------------------------- > > This could break only if a ser/deser sequence is interleaved with > another serialization event > > ----------------------------------------------------------------------------- > PH handle# IPC IPA handl# > ----------------------------------------------------------------------------- > ser(map) 1 ----> > > <---- ser(map) 1 > > deser(map) > max(1,1)=1 > max(1,1)=1 > deser(map) > > ser(map) 2 > ... > ----------------------------------------------------------------------------- > > Which is very unfortunate but possible. > > We have no centralized state between the two processes and we have no > way to assign handles in the proxies, as they have the same > information as the control serializer has. > > This might seem rather stupid, but we have two sides of the IPC, can't > we use even handles on one side and odd ones on the other ? The > proxies initialize the controls serializer with a seed (0 or 1) and we > simply serial_ += 2 ? That's very much a hack. A pretty clever one actually :-) With a comment to tell that this should likely be revisited (and a very brief explanation of why it's done that way), I think I'm fine with it. Paul, any comment ? > > > + > > > /* > > > * Use the ControlIdMap corresponding to the id map type. If the type > > > * references a globally defined id map (such as controls::controls
On Fri, Sep 03, 2021 at 09:11:30PM +0100, Kieran Bingham wrote: > On 03/09/2021 10:16, Jacopo Mondi wrote: > > On Thu, Sep 02, 2021 at 03:05:23PM +0100, Kieran Bingham wrote: > >> On 01/09/2021 15:37, Jacopo Mondi wrote: > >>> When running the IPA in isolated mode, each side of the IPC boundary > >>> has an instance of the ControlSerializer class which is used to > >>> serializer/deserialize controls before transmitting them on the wire. > >>> > >>> The IPAProxyWorker, which creates and manages the process the IPA runs into, > >>> does not reset its ControlSerializer upon an IPA::configure() call, while > >>> the IPAProxy does so, effectively creating a misalignment between the > >>> two sides of the fence. > >>> > >>> This obviously creates issues as one side of the IPC runs with a > >>> populated and possibly stale cache of ControlInfoMap references, while the > >>> other side gets reset every time a new configuration is applied to the > >>> Camera. > >>> > >>> Fix that by resetting the IPAProxyWorker ControlSerializer on an > >>> IPA configure() call. > >>> > >>> This change fixes an issue which is easily triggered by running two > >>> consecutive capture sessions with the IPA running in isolated mode: > >>> ERROR Serializer control_serializer.cpp:520 Can't deserialize ControlList: unknown ControlInfoMap > >>> > >>> Fixes: 7832e19a599e ("utils: ipc: add templates for code generation for IPC mechanism") > >> > >> Is it also technically fixing the patch that added the reset on the > >> other side? as that's when the misalignment begins? > > > > The 'other side' was reset from the very beginning. > > > > To be honest I would question the usage of Fixes in libcamera, as we > > don't have versions to backport to, but I added this mosotly for cargo > > cult. > > > > Iff we want to keep using Fixes I think that's the opportune commit to > > point to, as this patch should be ideally fixed-up in that one (if we > > could travel back in time) That's fine with me. > I don't think we have any requirement to add a Fixes tag, except it > helps promote awareness of /why/ or /what/ is being fixed. > > It also helps provide context in some occasions in case the fix turns > out to be wrong, so it's partly documentation. Imagine reading back > through the history trying to find out why something changed that > doesn't seem to make sense. Seeing a Fixes tag helps you see 'Oh, ok - > so this change was to fix that patch, so between there and here, the > thing isn't going to be right'. > > > And that it's likely a good habit to keep from Kernel side? There's probably a part of cargo-cult here, but I agree with Kieran that the information can be useful. As it's not costly to keep using Fixes tags, I'd rather keep them for reference. > But no, I don't think we're backporting to a non-existent version. :-) > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>> --- > >>> .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl | 4 ++++ > >>> 1 file changed, 4 insertions(+) > >>> > >>> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl > >>> index 8a88bd467da7..22cbc15c5eff 100644 > >>> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl > >>> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl > >>> @@ -79,6 +79,10 @@ public: > >>> > >>> {% for method in interface_main.methods %} > >>> case {{cmd_enum_name}}::{{method.mojom_name|cap}}: { > >>> + > >>> +{%- if method.mojom_name == "configure" %} > >>> + controlSerializer_.reset(); > >> > >> one more level indentation >> ? > >> > >> It looks like we're "inside" this case statement, so it should be one > >> level deeper. > >> > > > > Looking at the generated code > > > > case _IPU3Cmd::Configure: { > > controlSerializer_.reset(); > > > > > > const size_t configInfoStart = 0; > > > > > > IPAConfigInfo configInfo = IPADataSerializer<IPAConfigInfo>::deserialize( > > > > You are correct the indentation is wrong, but it does anyway matches > > the rest of the file :) > > Indeed, it matches the next two lines (A,B) - but the rest of the case > statement is otherwise correct. > > If I could figure out how to correct those two lines I would - but I > rabbit-holed too long in this template yesterday. > > I think it's still reasonable to put the controlSerializer_.reset(); at > the correct indentation. > > The args are incorrectly indented, but would be solved by fixing that > (waves hand magically) "somewhere else" ;-) : > > > case _IPU3Cmd::Configure: { > > > > > > A const size_t configInfoStart = 0; > > > > > > B IPAConfigInfo configInfo = IPADataSerializer<IPAConfigInfo>::deserialize( > > B _ipcMessage.data().cbegin() + configInfoStart, > > B _ipcMessage.data().cend(), > > B &controlSerializer_); > > > > > > int32_t _callRet =ipa_->configure(configInfo); > > > > IPCMessage::Header header = { _ipcMessage.header().cmd, _ipcMessage.header().cookie }; > > IPCMessage _response(header); > > std::vector<uint8_t> _callRetBuf; > > std::tie(_callRetBuf, std::ignore) = > > IPADataSerializer<int32_t>::serialize(_callRet); > > _response.data().insert(_response.data().end(), _callRetBuf.cbegin(), _callRetBuf.cend()); > > > > int _ret = socket_.send(_response.payload()); > > if (_ret < 0) { > > LOG(IPAProxyIPU3Worker, Error) > > << "Reply to configure() failed: " << _ret; > > } > > LOG(IPAProxyIPU3Worker, Debug) << "Done replying to configure()"; > > break; > > } > > Paul, Are A and B easy fixes? They appear to be automagically generated > to do with the IPADataSerializer for every call? > > >>> +{%- endif %} > >> > >> > >> Wow, these templates are hard to parse :-) > >> > >> But ... it looks like this is right ;-) > >> > >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> > >>> {{proxy_funcs.deserialize_call(method|method_param_inputs, '_ipcMessage.data()', '_ipcMessage.fds()', false, true)|indent(8, true)}} > >>> {% for param in method|method_param_outputs %} > >>> {{param|name}} {{param.mojom_name}};
Hi Kieran, On Fri, Sep 03, 2021 at 09:11:30PM +0100, Kieran Bingham wrote: > On 03/09/2021 10:16, Jacopo Mondi wrote: > > Hi Kieran, > > > > On Thu, Sep 02, 2021 at 03:05:23PM +0100, Kieran Bingham wrote: > >> On 01/09/2021 15:37, Jacopo Mondi wrote: > >>> When running the IPA in isolated mode, each side of the IPC boundary > >>> has an instance of the ControlSerializer class which is used to > >>> serializer/deserialize controls before transmitting them on the wire. > >>> > >>> The IPAProxyWorker, which creates and manages the process the IPA runs into, > >>> does not reset its ControlSerializer upon an IPA::configure() call, while > >>> the IPAProxy does so, effectively creating a misalignment between the > >>> two sides of the fence. > >>> > >>> This obviously creates issues as one side of the IPC runs with a > >>> populated and possibly stale cache of ControlInfoMap references, while the > >>> other side gets reset every time a new configuration is applied to the > >>> Camera. > >>> > >>> Fix that by resetting the IPAProxyWorker ControlSerializer on an > >>> IPA configure() call. > >>> > >>> This change fixes an issue which is easily triggered by running two > >>> consecutive capture sessions with the IPA running in isolated mode: > >>> ERROR Serializer control_serializer.cpp:520 Can't deserialize ControlList: unknown ControlInfoMap > >>> > >>> Fixes: 7832e19a599e ("utils: ipc: add templates for code generation for IPC mechanism") > >> > >> Is it also technically fixing the patch that added the reset on the > >> other side? as that's when the misalignment begins? > >> > > > > The 'other side' was reset from the very beginning. > > > > To be honest I would question the usage of Fixes in libcamera, as we > > don't have versions to backport to, but I added this mosotly for cargo > > cult. > > > > Iff we want to keep using Fixes I think that's the opportune commit to > > point to, as this patch should be ideally fixed-up in that one (if we > > could travel back in time) > > I don't think we have any requirement to add a Fixes tag, except it > helps promote awareness of /why/ or /what/ is being fixed. > > It also helps provide context in some occasions in case the fix turns > out to be wrong, so it's partly documentation. Imagine reading back > through the history trying to find out why something changed that > doesn't seem to make sense. Seeing a Fixes tag helps you see 'Oh, ok - > so this change was to fix that patch, so between there and here, the > thing isn't going to be right'. > > > And that it's likely a good habit to keep from Kernel side? > > > But no, I don't think we're backporting to a non-existent version. > > > > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>> --- > >>> .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl | 4 ++++ > >>> 1 file changed, 4 insertions(+) > >>> > >>> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl > >>> index 8a88bd467da7..22cbc15c5eff 100644 > >>> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl > >>> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl > >>> @@ -79,6 +79,10 @@ public: > >>> > >>> {% for method in interface_main.methods %} > >>> case {{cmd_enum_name}}::{{method.mojom_name|cap}}: { > >>> + > >>> +{%- if method.mojom_name == "configure" %} > >>> + controlSerializer_.reset(); > >> > >> one more level indentation >> ? > >> > >> It looks like we're "inside" this case statement, so it should be one > >> level deeper. > >> > > > > Looking at the generated code > > > > case _IPU3Cmd::Configure: { > > controlSerializer_.reset(); > > > > > > const size_t configInfoStart = 0; > > > > > > IPAConfigInfo configInfo = IPADataSerializer<IPAConfigInfo>::deserialize( > > > > You are correct the indentation is wrong, but it does anyway matches > > the rest of the file :) > > Indeed, it matches the next two lines (A,B) - but the rest of the case > statement is otherwise correct. > > If I could figure out how to correct those two lines I would - but I > rabbit-holed too long in this template yesterday. > > I think it's still reasonable to put the controlSerializer_.reset(); at > the correct indentation. > > The args are incorrectly indented, but would be solved by fixing that > (waves hand magically) "somewhere else" ;-) : > > > case _IPU3Cmd::Configure: { > > > > > > A const size_t configInfoStart = 0; > > > > > > B IPAConfigInfo configInfo = IPADataSerializer<IPAConfigInfo>::deserialize( > > B _ipcMessage.data().cbegin() + configInfoStart, > > B _ipcMessage.data().cend(), > > B &controlSerializer_); > > > > > > int32_t _callRet =ipa_->configure(configInfo); > > > > IPCMessage::Header header = { _ipcMessage.header().cmd, _ipcMessage.header().cookie }; > > IPCMessage _response(header); > > std::vector<uint8_t> _callRetBuf; > > std::tie(_callRetBuf, std::ignore) = > > IPADataSerializer<int32_t>::serialize(_callRet); > > _response.data().insert(_response.data().end(), _callRetBuf.cbegin(), _callRetBuf.cend()); > > > > int _ret = socket_.send(_response.payload()); > > if (_ret < 0) { > > LOG(IPAProxyIPU3Worker, Error) > > << "Reply to configure() failed: " << _ret; > > } > > LOG(IPAProxyIPU3Worker, Debug) << "Done replying to configure()"; > > break; > > } > > Paul, Are A and B easy fixes? They appear to be automagically generated > to do with the IPADataSerializer for every call? Yeah that should be easy. I'll send a patch. Paul > > >>> +{%- endif %} > >> > >> > >> Wow, these templates are hard to parse :-) > >> > >> But ... it looks like this is right ;-) > >> > >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> > >>> {{proxy_funcs.deserialize_call(method|method_param_inputs, '_ipcMessage.data()', '_ipcMessage.fds()', false, true)|indent(8, true)}} > >>> {% for param in method|method_param_outputs %} > >>> {{param|name}} {{param.mojom_name}}; > >>>
Hi Laurent and Jacopo, On Mon, Sep 06, 2021 at 02:01:30AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > On Fri, Sep 03, 2021 at 03:28:41PM +0200, Jacopo Mondi wrote: > > On Fri, Sep 03, 2021 at 02:46:35PM +0300, Laurent Pinchart wrote: > > > On Wed, Sep 01, 2021 at 04:37:57PM +0200, Jacopo Mondi wrote: > > > > When running the IPA in isolated mode, each side of the IPC boundary > > > > has an instance of the ControlSerializer class. > > > > > > > > Each serializer instance tags with a numerical id (handle) each > > > > ControlInfoMap instance it serializes, to be capable of associating > > > > ControlList with it at serialization and deserialization time, and > > > > increments the numerical counter for each newly serialized control info > > > > map. > > > > > > > > Having two instances of the ControlSerializer class running in two > > > > different process spaces, each instance increments its own counter, > > > > preventing from maintaining a globally shared state where each > > > > ControlInfoMap instance is reference by a unique identifier. > > > > > > > > Fix this by advancing the serial_ counter at ControlInfoMap > > > > de-serialization time, so that each newly serialized map will have a > > > > globally unique identifier. > > > > > > That's an interesting one. The control serializer was developed with the > > > assumption that a ControlInfoMap would only be sent from the pipeline > > > handler to the IPA, not the other way around. > > > > > > > Fixes: 2c5f0ad23aa4 ("libcamera: Add controls serializer") > > > > > > Technically, the breakage was introduced by the first change in the IPA > > > protocol that sent a ControlInfoMap in the IPA to pipeline handler > > > direction, but I don't mind keeping the Fixes tag as-is. > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > src/libcamera/control_serializer.cpp | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > > > > index 27360526f9eb..08cfecca3078 100644 > > > > --- a/src/libcamera/control_serializer.cpp > > > > +++ b/src/libcamera/control_serializer.cpp > > > > @@ -380,6 +380,9 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > > > > return {}; > > > > } > > > > > > > > + /* Keep the info map handles in sync between the two sides of IPC. */ > > > > + serial_ = std::max(serial_, hdr->handle); > > > > > > Can it be this simple ? What if the pipeline handler sends a > > > ControlInfoMap to the IPA at the same time as the IPA sends a > > > ControlInfoMap to the pipeline handler ? Each side will increment their > > > serial number, but the same number will refer to two different maps. > > > > Yes, this might happen during a capture session, where the PH/IPA > > interactions are aysnchronous. It can't happen before, as there won't > > be concurrect calls between the two sides of the IPC > > > > A regular run looks like > > > > ----------------------------------------------------------------------------- > > PH handle# IPC IPA handl# > > ----------------------------------------------------------------------------- > > ser(map) 1 ---> > > deser(map) > > max(0,1)=1 > > > > <--- ser(map) 2 > > max(1,2)=2 > > deser(map) > > > > ser(map) 3 > > ... > > ----------------------------------------------------------------------------- > > > > This could break only if a ser/deser sequence is interleaved with > > another serialization event > > > > ----------------------------------------------------------------------------- > > PH handle# IPC IPA handl# > > ----------------------------------------------------------------------------- > > ser(map) 1 ----> > > > > <---- ser(map) 1 > > > > deser(map) > > max(1,1)=1 > > max(1,1)=1 > > deser(map) > > > > ser(map) 2 > > ... > > ----------------------------------------------------------------------------- > > > > Which is very unfortunate but possible. > > > > We have no centralized state between the two processes and we have no > > way to assign handles in the proxies, as they have the same > > information as the control serializer has. > > > > This might seem rather stupid, but we have two sides of the IPC, can't > > we use even handles on one side and odd ones on the other ? The > > proxies initialize the controls serializer with a seed (0 or 1) and we > > simply serial_ += 2 ? > > That's very much a hack. A pretty clever one actually :-) With a comment > to tell that this should likely be revisited (and a very brief > explanation of why it's done that way), I think I'm fine with it. Paul, > any comment ? Ah, I see, you separate the serial number spaces, that way it'll never conflict. I think that's a good idea. Paul > > > > > + > > > > /* > > > > * Use the ControlIdMap corresponding to the id map type. If the type > > > > * references a globally defined id map (such as controls::controls > > -- > Regards, > > Laurent Pinchart