| Message ID | 20250811131159.1342981-1-barnabas.pocze@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Barnabás Pőcze (2025-08-11 14:11:59) > Aliasing an arbitrary buffer with some `T *` is unlikely to yield good > results, primarily due to potential alignment issues. So don't do it, > instead create the objects on the stack and read into them, this > removes any and all strict aliasing or alignment concerns. > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=221 > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/libcamera/control_serializer.cpp | 75 +++++++++++++--------------- > 1 file changed, 36 insertions(+), 39 deletions(-) > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > index 050f8512b..722ff231c 100644 > --- a/src/libcamera/control_serializer.cpp > +++ b/src/libcamera/control_serializer.cpp > @@ -427,22 +427,22 @@ ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b) > template<> > ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &buffer) > { > - const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); > - if (!hdr) { > + struct ipa_controls_header hdr; > + if (buffer.read(&hdr) < 0) { Aha, but the key difference is that before it was 'zero copy' just pointing at the data, while now we're making a copy ? I wonder if we can detect the alignment and only do the copy if unaligned? Or maybe that's just not worth it and we should just accept the copy - this shouldn't be a high data volume for the most parts... But maybe its something we want to be able to profile/monitor in the future. Or perhaps can we find a way to align data when it goes into the ByteStreamBuffer with some alignment padding sections somehow? If fixing/specifying the alignment in the buffer isn't possible easy - then I could certainly imagine accepting this as a current possible solution ... > LOG(Serializer, Error) << "Out of data"; > return {}; > } > > - auto iter = infoMaps_.find(hdr->handle); > + auto iter = infoMaps_.find(hdr.handle); > if (iter != infoMaps_.end()) { > LOG(Serializer, Debug) << "Use cached ControlInfoMap"; > return iter->second; > } > > - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { > + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { > LOG(Serializer, Error) > << "Unsupported controls format version " > - << hdr->version; > + << hdr.version; > return {}; > } > > @@ -455,7 +455,7 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > */ > const ControlIdMap *idMap = nullptr; > ControlIdMap *localIdMap = nullptr; > - switch (hdr->id_map_type) { > + switch (hdr.id_map_type) { > case IPA_CONTROL_ID_MAP_CONTROLS: > idMap = &controls::controls; > break; > @@ -469,12 +469,12 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > break; > default: > LOG(Serializer, Error) > - << "Unknown id map type: " << hdr->id_map_type; > + << "Unknown id map type: " << hdr.id_map_type; > return {}; > } > > - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); > - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); > + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); > + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); > > if (buffer.overflow()) { > LOG(Serializer, Error) << "Out of data"; > @@ -482,36 +482,35 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > } > > ControlInfoMap::Map ctrls; > - for (unsigned int i = 0; i < hdr->entries; ++i) { > - const struct ipa_control_info_entry *entry = > - entries.read<decltype(*entry)>(); > - if (!entry) { > + for (unsigned int i = 0; i < hdr.entries; ++i) { > + struct ipa_control_info_entry entry; > + if (entries.read(&entry) < 0) { > LOG(Serializer, Error) << "Out of data"; > return {}; > } > > - ControlType type = static_cast<ControlType>(entry->type); > + ControlType type = static_cast<ControlType>(entry.type); > > /* If we're using a local id map, populate it. */ > if (localIdMap) { > ControlId::DirectionFlags flags{ > - static_cast<ControlId::Direction>(entry->direction) > + static_cast<ControlId::Direction>(entry.direction) > }; > > /** > * \todo Find a way to preserve the control name for > * debugging purpose. > */ > - controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, > + controlIds_.emplace_back(std::make_unique<ControlId>(entry.id, > "", "local", type, > flags)); > - (*localIdMap)[entry->id] = controlIds_.back().get(); > + (*localIdMap)[entry.id] = controlIds_.back().get(); > } > > - const ControlId *controlId = idMap->at(entry->id); > + const ControlId *controlId = idMap->at(entry.id); > ASSERT(controlId); > > - if (entry->offset != values.offset()) { > + if (entry.offset != values.offset()) { > LOG(Serializer, Error) > << "Bad data, entry offset mismatch (entry " > << i << ")"; > @@ -526,9 +525,9 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > * Create the ControlInfoMap in the cache, and store the map to handle > * association. > */ > - infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap); > - ControlInfoMap &map = infoMaps_[hdr->handle]; > - infoMapHandles_[&map] = hdr->handle; > + infoMaps_[hdr.handle] = ControlInfoMap(std::move(ctrls), *idMap); > + ControlInfoMap &map = infoMaps_[hdr.handle]; > + infoMapHandles_[&map] = hdr.handle; > > return map; > } > @@ -545,21 +544,21 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > template<> > ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer) > { > - const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); > - if (!hdr) { > + struct ipa_controls_header hdr; > + if (buffer.read(&hdr) < 0) { > LOG(Serializer, Error) << "Out of data"; > return {}; > } > > - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { > + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { > LOG(Serializer, Error) > << "Unsupported controls format version " > - << hdr->version; > + << hdr.version; > return {}; > } > > - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); > - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); > + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); > + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); > > if (buffer.overflow()) { > LOG(Serializer, Error) << "Out of data"; > @@ -576,10 +575,10 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > * ControlInfoMap is available. > */ > const ControlIdMap *idMap; > - if (hdr->handle) { > + if (hdr.handle) { > auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(), > [&](decltype(infoMapHandles_)::value_type &entry) { > - return entry.second == hdr->handle; > + return entry.second == hdr.handle; > }); > if (iter == infoMapHandles_.end()) { > LOG(Serializer, Error) > @@ -590,7 +589,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > const ControlInfoMap *infoMap = iter->first; > idMap = &infoMap->idmap(); > } else { > - switch (hdr->id_map_type) { > + switch (hdr.id_map_type) { > case IPA_CONTROL_ID_MAP_CONTROLS: > idMap = &controls::controls; > break; > @@ -601,7 +600,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > > case IPA_CONTROL_ID_MAP_V4L2: > default: > - if (hdr->entries > 0) > + if (hdr.entries > 0) > LOG(Serializer, Fatal) > << "A list of V4L2 controls requires a ControlInfoMap"; > > @@ -617,23 +616,21 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > */ > ControlList ctrls(*idMap); > > - for (unsigned int i = 0; i < hdr->entries; ++i) { > - const struct ipa_control_value_entry *entry = > - entries.read<decltype(*entry)>(); > - if (!entry) { > + for (unsigned int i = 0; i < hdr.entries; ++i) { > + struct ipa_control_value_entry entry; > + if (entries.read(&entry) < 0) { > LOG(Serializer, Error) << "Out of data"; > return {}; > } > > - if (entry->offset != values.offset()) { > + if (entry.offset != values.offset()) { > LOG(Serializer, Error) > << "Bad data, entry offset mismatch (entry " > << i << ")"; > return {}; > } > > - ctrls.set(entry->id, > - loadControlValue(values, entry->is_array, entry->count)); > + ctrls.set(entry.id, loadControlValue(values, entry.is_array, entry.count)); > } > > return ctrls; > -- > 2.50.1 >
Hi 2025. 08. 11. 16:12 keltezéssel, Kieran Bingham írta: > Quoting Barnabás Pőcze (2025-08-11 14:11:59) >> Aliasing an arbitrary buffer with some `T *` is unlikely to yield good >> results, primarily due to potential alignment issues. So don't do it, >> instead create the objects on the stack and read into them, this >> removes any and all strict aliasing or alignment concerns. >> >> Bug: https://bugs.libcamera.org/show_bug.cgi?id=221 >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> src/libcamera/control_serializer.cpp | 75 +++++++++++++--------------- >> 1 file changed, 36 insertions(+), 39 deletions(-) >> >> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp >> index 050f8512b..722ff231c 100644 >> --- a/src/libcamera/control_serializer.cpp >> +++ b/src/libcamera/control_serializer.cpp >> @@ -427,22 +427,22 @@ ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b) >> template<> >> ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &buffer) >> { >> - const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); >> - if (!hdr) { >> + struct ipa_controls_header hdr; >> + if (buffer.read(&hdr) < 0) { > > Aha, but the key difference is that before it was 'zero copy' just > pointing at the data, while now we're making a copy ? Yes. > > I wonder if we can detect the alignment and only do the copy if > unaligned? Or maybe that's just not worth it and we should just accept > the copy - this shouldn't be a high data volume for the most parts... I feel like the control id/info map/list/value allocation easily dominates the cost of these small copies. These are all <=32 bytes. > > But maybe its something we want to be able to profile/monitor in the > future. > > > Or perhaps can we find a way to align data when it goes into the > ByteStreamBuffer with some alignment padding sections somehow? I could be missing something, but given how ipc serialization is done at the moment, it does not seem entirely trivial. > > > If fixing/specifying the alignment in the buffer isn't possible easy - > then I could certainly imagine accepting this as a current possible solution ... > >> LOG(Serializer, Error) << "Out of data"; >> return {}; >> } >> >> - auto iter = infoMaps_.find(hdr->handle); >> + auto iter = infoMaps_.find(hdr.handle); >> if (iter != infoMaps_.end()) { >> LOG(Serializer, Debug) << "Use cached ControlInfoMap"; >> return iter->second; >> } >> >> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { >> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { >> LOG(Serializer, Error) >> << "Unsupported controls format version " >> - << hdr->version; >> + << hdr.version; >> return {}; >> } >> >> @@ -455,7 +455,7 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & >> */ >> const ControlIdMap *idMap = nullptr; >> ControlIdMap *localIdMap = nullptr; >> - switch (hdr->id_map_type) { >> + switch (hdr.id_map_type) { >> case IPA_CONTROL_ID_MAP_CONTROLS: >> idMap = &controls::controls; >> break; >> @@ -469,12 +469,12 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & >> break; >> default: >> LOG(Serializer, Error) >> - << "Unknown id map type: " << hdr->id_map_type; >> + << "Unknown id map type: " << hdr.id_map_type; >> return {}; >> } >> >> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); >> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); >> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); >> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); >> >> if (buffer.overflow()) { >> LOG(Serializer, Error) << "Out of data"; >> @@ -482,36 +482,35 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & >> } >> >> ControlInfoMap::Map ctrls; >> - for (unsigned int i = 0; i < hdr->entries; ++i) { >> - const struct ipa_control_info_entry *entry = >> - entries.read<decltype(*entry)>(); >> - if (!entry) { >> + for (unsigned int i = 0; i < hdr.entries; ++i) { >> + struct ipa_control_info_entry entry; >> + if (entries.read(&entry) < 0) { >> LOG(Serializer, Error) << "Out of data"; >> return {}; >> } >> >> - ControlType type = static_cast<ControlType>(entry->type); >> + ControlType type = static_cast<ControlType>(entry.type); >> >> /* If we're using a local id map, populate it. */ >> if (localIdMap) { >> ControlId::DirectionFlags flags{ >> - static_cast<ControlId::Direction>(entry->direction) >> + static_cast<ControlId::Direction>(entry.direction) >> }; >> >> /** >> * \todo Find a way to preserve the control name for >> * debugging purpose. >> */ >> - controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, >> + controlIds_.emplace_back(std::make_unique<ControlId>(entry.id, >> "", "local", type, >> flags)); >> - (*localIdMap)[entry->id] = controlIds_.back().get(); >> + (*localIdMap)[entry.id] = controlIds_.back().get(); >> } >> >> - const ControlId *controlId = idMap->at(entry->id); >> + const ControlId *controlId = idMap->at(entry.id); >> ASSERT(controlId); >> >> - if (entry->offset != values.offset()) { >> + if (entry.offset != values.offset()) { >> LOG(Serializer, Error) >> << "Bad data, entry offset mismatch (entry " >> << i << ")"; >> @@ -526,9 +525,9 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & >> * Create the ControlInfoMap in the cache, and store the map to handle >> * association. >> */ >> - infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap); >> - ControlInfoMap &map = infoMaps_[hdr->handle]; >> - infoMapHandles_[&map] = hdr->handle; >> + infoMaps_[hdr.handle] = ControlInfoMap(std::move(ctrls), *idMap); >> + ControlInfoMap &map = infoMaps_[hdr.handle]; >> + infoMapHandles_[&map] = hdr.handle; >> >> return map; >> } >> @@ -545,21 +544,21 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & >> template<> >> ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer) >> { >> - const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); >> - if (!hdr) { >> + struct ipa_controls_header hdr; >> + if (buffer.read(&hdr) < 0) { >> LOG(Serializer, Error) << "Out of data"; >> return {}; >> } >> >> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { >> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { >> LOG(Serializer, Error) >> << "Unsupported controls format version " >> - << hdr->version; >> + << hdr.version; >> return {}; >> } >> >> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); >> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); >> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); >> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); >> >> if (buffer.overflow()) { >> LOG(Serializer, Error) << "Out of data"; >> @@ -576,10 +575,10 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer >> * ControlInfoMap is available. >> */ >> const ControlIdMap *idMap; >> - if (hdr->handle) { >> + if (hdr.handle) { >> auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(), >> [&](decltype(infoMapHandles_)::value_type &entry) { >> - return entry.second == hdr->handle; >> + return entry.second == hdr.handle; >> }); >> if (iter == infoMapHandles_.end()) { >> LOG(Serializer, Error) >> @@ -590,7 +589,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer >> const ControlInfoMap *infoMap = iter->first; >> idMap = &infoMap->idmap(); >> } else { >> - switch (hdr->id_map_type) { >> + switch (hdr.id_map_type) { >> case IPA_CONTROL_ID_MAP_CONTROLS: >> idMap = &controls::controls; >> break; >> @@ -601,7 +600,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer >> >> case IPA_CONTROL_ID_MAP_V4L2: >> default: >> - if (hdr->entries > 0) >> + if (hdr.entries > 0) >> LOG(Serializer, Fatal) >> << "A list of V4L2 controls requires a ControlInfoMap"; >> >> @@ -617,23 +616,21 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer >> */ >> ControlList ctrls(*idMap); >> >> - for (unsigned int i = 0; i < hdr->entries; ++i) { >> - const struct ipa_control_value_entry *entry = >> - entries.read<decltype(*entry)>(); >> - if (!entry) { >> + for (unsigned int i = 0; i < hdr.entries; ++i) { >> + struct ipa_control_value_entry entry; >> + if (entries.read(&entry) < 0) { >> LOG(Serializer, Error) << "Out of data"; >> return {}; >> } >> >> - if (entry->offset != values.offset()) { >> + if (entry.offset != values.offset()) { >> LOG(Serializer, Error) >> << "Bad data, entry offset mismatch (entry " >> << i << ")"; >> return {}; >> } >> >> - ctrls.set(entry->id, >> - loadControlValue(values, entry->is_array, entry->count)); >> + ctrls.set(entry.id, loadControlValue(values, entry.is_array, entry.count)); >> } >> >> return ctrls; >> -- >> 2.50.1 >>
Quoting Barnabás Pőcze (2025-08-11 15:18:57) > Hi > > 2025. 08. 11. 16:12 keltezéssel, Kieran Bingham írta: > > Quoting Barnabás Pőcze (2025-08-11 14:11:59) > >> Aliasing an arbitrary buffer with some `T *` is unlikely to yield good > >> results, primarily due to potential alignment issues. So don't do it, > >> instead create the objects on the stack and read into them, this > >> removes any and all strict aliasing or alignment concerns. > >> > >> Bug: https://bugs.libcamera.org/show_bug.cgi?id=221 > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >> --- > >> src/libcamera/control_serializer.cpp | 75 +++++++++++++--------------- > >> 1 file changed, 36 insertions(+), 39 deletions(-) > >> > >> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > >> index 050f8512b..722ff231c 100644 > >> --- a/src/libcamera/control_serializer.cpp > >> +++ b/src/libcamera/control_serializer.cpp > >> @@ -427,22 +427,22 @@ ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b) > >> template<> > >> ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &buffer) > >> { > >> - const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); > >> - if (!hdr) { > >> + struct ipa_controls_header hdr; > >> + if (buffer.read(&hdr) < 0) { > > > > Aha, but the key difference is that before it was 'zero copy' just > > pointing at the data, while now we're making a copy ? > > Yes. > > > > > > I wonder if we can detect the alignment and only do the copy if > > unaligned? Or maybe that's just not worth it and we should just accept > > the copy - this shouldn't be a high data volume for the most parts... > > I feel like the control id/info map/list/value allocation easily dominates the > cost of these small copies. These are all <=32 bytes. Hrm.. indeed - we don't normally expect ControlLists to contain large data. I thnik CCMs with a 3x3 matrix is 'hopefully' the biggest we expect for the most part and things like lens shading tables shouldn't be popultated through a control list. So ... yes a copy seems quick and trivial... I'll put this in already: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> But probably want this also checked by either or both of Laurent and Paul who know the IPC mechanisms well. -- Kieran > > > > > > But maybe its something we want to be able to profile/monitor in the > > future. > > > > > > Or perhaps can we find a way to align data when it goes into the > > ByteStreamBuffer with some alignment padding sections somehow? > > I could be missing something, but given how ipc serialization is done > at the moment, it does not seem entirely trivial. > > > > > > > > If fixing/specifying the alignment in the buffer isn't possible easy - > > then I could certainly imagine accepting this as a current possible solution ... > > > >> LOG(Serializer, Error) << "Out of data"; > >> return {}; > >> } > >> > >> - auto iter = infoMaps_.find(hdr->handle); > >> + auto iter = infoMaps_.find(hdr.handle); > >> if (iter != infoMaps_.end()) { > >> LOG(Serializer, Debug) << "Use cached ControlInfoMap"; > >> return iter->second; > >> } > >> > >> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { > >> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { > >> LOG(Serializer, Error) > >> << "Unsupported controls format version " > >> - << hdr->version; > >> + << hdr.version; > >> return {}; > >> } > >> > >> @@ -455,7 +455,7 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > >> */ > >> const ControlIdMap *idMap = nullptr; > >> ControlIdMap *localIdMap = nullptr; > >> - switch (hdr->id_map_type) { > >> + switch (hdr.id_map_type) { > >> case IPA_CONTROL_ID_MAP_CONTROLS: > >> idMap = &controls::controls; > >> break; > >> @@ -469,12 +469,12 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > >> break; > >> default: > >> LOG(Serializer, Error) > >> - << "Unknown id map type: " << hdr->id_map_type; > >> + << "Unknown id map type: " << hdr.id_map_type; > >> return {}; > >> } > >> > >> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); > >> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); > >> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); > >> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); > >> > >> if (buffer.overflow()) { > >> LOG(Serializer, Error) << "Out of data"; > >> @@ -482,36 +482,35 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > >> } > >> > >> ControlInfoMap::Map ctrls; > >> - for (unsigned int i = 0; i < hdr->entries; ++i) { > >> - const struct ipa_control_info_entry *entry = > >> - entries.read<decltype(*entry)>(); > >> - if (!entry) { > >> + for (unsigned int i = 0; i < hdr.entries; ++i) { > >> + struct ipa_control_info_entry entry; > >> + if (entries.read(&entry) < 0) { > >> LOG(Serializer, Error) << "Out of data"; > >> return {}; > >> } > >> > >> - ControlType type = static_cast<ControlType>(entry->type); > >> + ControlType type = static_cast<ControlType>(entry.type); > >> > >> /* If we're using a local id map, populate it. */ > >> if (localIdMap) { > >> ControlId::DirectionFlags flags{ > >> - static_cast<ControlId::Direction>(entry->direction) > >> + static_cast<ControlId::Direction>(entry.direction) > >> }; > >> > >> /** > >> * \todo Find a way to preserve the control name for > >> * debugging purpose. > >> */ > >> - controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, > >> + controlIds_.emplace_back(std::make_unique<ControlId>(entry.id, > >> "", "local", type, > >> flags)); > >> - (*localIdMap)[entry->id] = controlIds_.back().get(); > >> + (*localIdMap)[entry.id] = controlIds_.back().get(); > >> } > >> > >> - const ControlId *controlId = idMap->at(entry->id); > >> + const ControlId *controlId = idMap->at(entry.id); > >> ASSERT(controlId); > >> > >> - if (entry->offset != values.offset()) { > >> + if (entry.offset != values.offset()) { > >> LOG(Serializer, Error) > >> << "Bad data, entry offset mismatch (entry " > >> << i << ")"; > >> @@ -526,9 +525,9 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > >> * Create the ControlInfoMap in the cache, and store the map to handle > >> * association. > >> */ > >> - infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap); > >> - ControlInfoMap &map = infoMaps_[hdr->handle]; > >> - infoMapHandles_[&map] = hdr->handle; > >> + infoMaps_[hdr.handle] = ControlInfoMap(std::move(ctrls), *idMap); > >> + ControlInfoMap &map = infoMaps_[hdr.handle]; > >> + infoMapHandles_[&map] = hdr.handle; > >> > >> return map; > >> } > >> @@ -545,21 +544,21 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > >> template<> > >> ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer) > >> { > >> - const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); > >> - if (!hdr) { > >> + struct ipa_controls_header hdr; > >> + if (buffer.read(&hdr) < 0) { > >> LOG(Serializer, Error) << "Out of data"; > >> return {}; > >> } > >> > >> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { > >> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { > >> LOG(Serializer, Error) > >> << "Unsupported controls format version " > >> - << hdr->version; > >> + << hdr.version; > >> return {}; > >> } > >> > >> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); > >> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); > >> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); > >> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); > >> > >> if (buffer.overflow()) { > >> LOG(Serializer, Error) << "Out of data"; > >> @@ -576,10 +575,10 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > >> * ControlInfoMap is available. > >> */ > >> const ControlIdMap *idMap; > >> - if (hdr->handle) { > >> + if (hdr.handle) { > >> auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(), > >> [&](decltype(infoMapHandles_)::value_type &entry) { > >> - return entry.second == hdr->handle; > >> + return entry.second == hdr.handle; > >> }); > >> if (iter == infoMapHandles_.end()) { > >> LOG(Serializer, Error) > >> @@ -590,7 +589,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > >> const ControlInfoMap *infoMap = iter->first; > >> idMap = &infoMap->idmap(); > >> } else { > >> - switch (hdr->id_map_type) { > >> + switch (hdr.id_map_type) { > >> case IPA_CONTROL_ID_MAP_CONTROLS: > >> idMap = &controls::controls; > >> break; > >> @@ -601,7 +600,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > >> > >> case IPA_CONTROL_ID_MAP_V4L2: > >> default: > >> - if (hdr->entries > 0) > >> + if (hdr.entries > 0) > >> LOG(Serializer, Fatal) > >> << "A list of V4L2 controls requires a ControlInfoMap"; > >> > >> @@ -617,23 +616,21 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > >> */ > >> ControlList ctrls(*idMap); > >> > >> - for (unsigned int i = 0; i < hdr->entries; ++i) { > >> - const struct ipa_control_value_entry *entry = > >> - entries.read<decltype(*entry)>(); > >> - if (!entry) { > >> + for (unsigned int i = 0; i < hdr.entries; ++i) { > >> + struct ipa_control_value_entry entry; > >> + if (entries.read(&entry) < 0) { > >> LOG(Serializer, Error) << "Out of data"; > >> return {}; > >> } > >> > >> - if (entry->offset != values.offset()) { > >> + if (entry.offset != values.offset()) { > >> LOG(Serializer, Error) > >> << "Bad data, entry offset mismatch (entry " > >> << i << ")"; > >> return {}; > >> } > >> > >> - ctrls.set(entry->id, > >> - loadControlValue(values, entry->is_array, entry->count)); > >> + ctrls.set(entry.id, loadControlValue(values, entry.is_array, entry.count)); > >> } > >> > >> return ctrls; > >> -- > >> 2.50.1 > >> >
2025. 08. 11. 16:23 keltezéssel, Kieran Bingham írta: > Quoting Barnabás Pőcze (2025-08-11 15:18:57) >> Hi >> >> 2025. 08. 11. 16:12 keltezéssel, Kieran Bingham írta: >>> Quoting Barnabás Pőcze (2025-08-11 14:11:59) >>>> Aliasing an arbitrary buffer with some `T *` is unlikely to yield good >>>> results, primarily due to potential alignment issues. So don't do it, >>>> instead create the objects on the stack and read into them, this >>>> removes any and all strict aliasing or alignment concerns. >>>> >>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=221 >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>> --- >>>> src/libcamera/control_serializer.cpp | 75 +++++++++++++--------------- >>>> 1 file changed, 36 insertions(+), 39 deletions(-) >>>> >>>> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp >>>> index 050f8512b..722ff231c 100644 >>>> --- a/src/libcamera/control_serializer.cpp >>>> +++ b/src/libcamera/control_serializer.cpp >>>> @@ -427,22 +427,22 @@ ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b) >>>> template<> >>>> ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &buffer) >>>> { >>>> - const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); >>>> - if (!hdr) { >>>> + struct ipa_controls_header hdr; >>>> + if (buffer.read(&hdr) < 0) { >>> >>> Aha, but the key difference is that before it was 'zero copy' just >>> pointing at the data, while now we're making a copy ? >> >> Yes. >> >> >>> >>> I wonder if we can detect the alignment and only do the copy if >>> unaligned? Or maybe that's just not worth it and we should just accept >>> the copy - this shouldn't be a high data volume for the most parts... >> >> I feel like the control id/info map/list/value allocation easily dominates the >> cost of these small copies. These are all <=32 bytes. > > Hrm.. indeed - we don't normally expect ControlLists to contain large > data. > > I thnik CCMs with a 3x3 matrix is 'hopefully' the biggest we expect for > the most part and things like lens shading tables shouldn't be > popultated through a control list. This change only affects the headers, e.g. the bytes of the control value data is still read the same way, directly from the buffer. > > So ... yes a copy seems quick and trivial... > > I'll put this in already: > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > But probably want this also checked by either or both of Laurent and > Paul who know the IPC mechanisms well. > > -- > Kieran > > >> >> >>> >>> But maybe its something we want to be able to profile/monitor in the >>> future. >>> >>> >>> Or perhaps can we find a way to align data when it goes into the >>> ByteStreamBuffer with some alignment padding sections somehow? >> >> I could be missing something, but given how ipc serialization is done >> at the moment, it does not seem entirely trivial. >> >> >>> >>> >>> If fixing/specifying the alignment in the buffer isn't possible easy - >>> then I could certainly imagine accepting this as a current possible solution ... >>> >>>> LOG(Serializer, Error) << "Out of data"; >>>> return {}; >>>> } >>>> >>>> - auto iter = infoMaps_.find(hdr->handle); >>>> + auto iter = infoMaps_.find(hdr.handle); >>>> if (iter != infoMaps_.end()) { >>>> LOG(Serializer, Debug) << "Use cached ControlInfoMap"; >>>> return iter->second; >>>> } >>>> >>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { >>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { >>>> LOG(Serializer, Error) >>>> << "Unsupported controls format version " >>>> - << hdr->version; >>>> + << hdr.version; >>>> return {}; >>>> } >>>> >>>> @@ -455,7 +455,7 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & >>>> */ >>>> const ControlIdMap *idMap = nullptr; >>>> ControlIdMap *localIdMap = nullptr; >>>> - switch (hdr->id_map_type) { >>>> + switch (hdr.id_map_type) { >>>> case IPA_CONTROL_ID_MAP_CONTROLS: >>>> idMap = &controls::controls; >>>> break; >>>> @@ -469,12 +469,12 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & >>>> break; >>>> default: >>>> LOG(Serializer, Error) >>>> - << "Unknown id map type: " << hdr->id_map_type; >>>> + << "Unknown id map type: " << hdr.id_map_type; >>>> return {}; >>>> } >>>> >>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); >>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); >>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); >>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); >>>> >>>> if (buffer.overflow()) { >>>> LOG(Serializer, Error) << "Out of data"; >>>> @@ -482,36 +482,35 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & >>>> } >>>> >>>> ControlInfoMap::Map ctrls; >>>> - for (unsigned int i = 0; i < hdr->entries; ++i) { >>>> - const struct ipa_control_info_entry *entry = >>>> - entries.read<decltype(*entry)>(); >>>> - if (!entry) { >>>> + for (unsigned int i = 0; i < hdr.entries; ++i) { >>>> + struct ipa_control_info_entry entry; >>>> + if (entries.read(&entry) < 0) { >>>> LOG(Serializer, Error) << "Out of data"; >>>> return {}; >>>> } >>>> >>>> - ControlType type = static_cast<ControlType>(entry->type); >>>> + ControlType type = static_cast<ControlType>(entry.type); >>>> >>>> /* If we're using a local id map, populate it. */ >>>> if (localIdMap) { >>>> ControlId::DirectionFlags flags{ >>>> - static_cast<ControlId::Direction>(entry->direction) >>>> + static_cast<ControlId::Direction>(entry.direction) >>>> }; >>>> >>>> /** >>>> * \todo Find a way to preserve the control name for >>>> * debugging purpose. >>>> */ >>>> - controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, >>>> + controlIds_.emplace_back(std::make_unique<ControlId>(entry.id, >>>> "", "local", type, >>>> flags)); >>>> - (*localIdMap)[entry->id] = controlIds_.back().get(); >>>> + (*localIdMap)[entry.id] = controlIds_.back().get(); >>>> } >>>> >>>> - const ControlId *controlId = idMap->at(entry->id); >>>> + const ControlId *controlId = idMap->at(entry.id); >>>> ASSERT(controlId); >>>> >>>> - if (entry->offset != values.offset()) { >>>> + if (entry.offset != values.offset()) { >>>> LOG(Serializer, Error) >>>> << "Bad data, entry offset mismatch (entry " >>>> << i << ")"; >>>> @@ -526,9 +525,9 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & >>>> * Create the ControlInfoMap in the cache, and store the map to handle >>>> * association. >>>> */ >>>> - infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap); >>>> - ControlInfoMap &map = infoMaps_[hdr->handle]; >>>> - infoMapHandles_[&map] = hdr->handle; >>>> + infoMaps_[hdr.handle] = ControlInfoMap(std::move(ctrls), *idMap); >>>> + ControlInfoMap &map = infoMaps_[hdr.handle]; >>>> + infoMapHandles_[&map] = hdr.handle; >>>> >>>> return map; >>>> } >>>> @@ -545,21 +544,21 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & >>>> template<> >>>> ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer) >>>> { >>>> - const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); >>>> - if (!hdr) { >>>> + struct ipa_controls_header hdr; >>>> + if (buffer.read(&hdr) < 0) { >>>> LOG(Serializer, Error) << "Out of data"; >>>> return {}; >>>> } >>>> >>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { >>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { >>>> LOG(Serializer, Error) >>>> << "Unsupported controls format version " >>>> - << hdr->version; >>>> + << hdr.version; >>>> return {}; >>>> } >>>> >>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); >>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); >>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); >>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); >>>> >>>> if (buffer.overflow()) { >>>> LOG(Serializer, Error) << "Out of data"; >>>> @@ -576,10 +575,10 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer >>>> * ControlInfoMap is available. >>>> */ >>>> const ControlIdMap *idMap; >>>> - if (hdr->handle) { >>>> + if (hdr.handle) { >>>> auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(), >>>> [&](decltype(infoMapHandles_)::value_type &entry) { >>>> - return entry.second == hdr->handle; >>>> + return entry.second == hdr.handle; >>>> }); >>>> if (iter == infoMapHandles_.end()) { >>>> LOG(Serializer, Error) >>>> @@ -590,7 +589,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer >>>> const ControlInfoMap *infoMap = iter->first; >>>> idMap = &infoMap->idmap(); >>>> } else { >>>> - switch (hdr->id_map_type) { >>>> + switch (hdr.id_map_type) { >>>> case IPA_CONTROL_ID_MAP_CONTROLS: >>>> idMap = &controls::controls; >>>> break; >>>> @@ -601,7 +600,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer >>>> >>>> case IPA_CONTROL_ID_MAP_V4L2: >>>> default: >>>> - if (hdr->entries > 0) >>>> + if (hdr.entries > 0) >>>> LOG(Serializer, Fatal) >>>> << "A list of V4L2 controls requires a ControlInfoMap"; >>>> >>>> @@ -617,23 +616,21 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer >>>> */ >>>> ControlList ctrls(*idMap); >>>> >>>> - for (unsigned int i = 0; i < hdr->entries; ++i) { >>>> - const struct ipa_control_value_entry *entry = >>>> - entries.read<decltype(*entry)>(); >>>> - if (!entry) { >>>> + for (unsigned int i = 0; i < hdr.entries; ++i) { >>>> + struct ipa_control_value_entry entry; >>>> + if (entries.read(&entry) < 0) { >>>> LOG(Serializer, Error) << "Out of data"; >>>> return {}; >>>> } >>>> >>>> - if (entry->offset != values.offset()) { >>>> + if (entry.offset != values.offset()) { >>>> LOG(Serializer, Error) >>>> << "Bad data, entry offset mismatch (entry " >>>> << i << ")"; >>>> return {}; >>>> } >>>> >>>> - ctrls.set(entry->id, >>>> - loadControlValue(values, entry->is_array, entry->count)); >>>> + ctrls.set(entry.id, loadControlValue(values, entry.is_array, entry.count)); >>>> } >>>> >>>> return ctrls; >>>> -- >>>> 2.50.1 >>>> >>
On Mon, Aug 11, 2025 at 04:28:10PM +0200, Barnabás Pőcze wrote: > 2025. 08. 11. 16:23 keltezéssel, Kieran Bingham írta: > > Quoting Barnabás Pőcze (2025-08-11 15:18:57) > >> 2025. 08. 11. 16:12 keltezéssel, Kieran Bingham írta: > >>> Quoting Barnabás Pőcze (2025-08-11 14:11:59) > >>>> Aliasing an arbitrary buffer with some `T *` is unlikely to yield good > >>>> results, primarily due to potential alignment issues. So don't do it, > >>>> instead create the objects on the stack and read into them, this > >>>> removes any and all strict aliasing or alignment concerns. > >>>> > >>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=221 > >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >>>> --- > >>>> src/libcamera/control_serializer.cpp | 75 +++++++++++++--------------- > >>>> 1 file changed, 36 insertions(+), 39 deletions(-) > >>>> > >>>> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > >>>> index 050f8512b..722ff231c 100644 > >>>> --- a/src/libcamera/control_serializer.cpp > >>>> +++ b/src/libcamera/control_serializer.cpp > >>>> @@ -427,22 +427,22 @@ ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b) > >>>> template<> > >>>> ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &buffer) > >>>> { > >>>> - const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); > >>>> - if (!hdr) { > >>>> + struct ipa_controls_header hdr; > >>>> + if (buffer.read(&hdr) < 0) { > >>> > >>> Aha, but the key difference is that before it was 'zero copy' just > >>> pointing at the data, while now we're making a copy ? > >> > >> Yes. > >> > >>> I wonder if we can detect the alignment and only do the copy if > >>> unaligned? Or maybe that's just not worth it and we should just accept > >>> the copy - this shouldn't be a high data volume for the most parts... > >> > >> I feel like the control id/info map/list/value allocation easily dominates the > >> cost of these small copies. These are all <=32 bytes. > > > > Hrm.. indeed - we don't normally expect ControlLists to contain large > > data. > > > > I thnik CCMs with a 3x3 matrix is 'hopefully' the biggest we expect for > > the most part and things like lens shading tables shouldn't be > > popultated through a control list. > > This change only affects the headers, e.g. the bytes of the control value > data is still read the same way, directly from the buffer. > > > So ... yes a copy seems quick and trivial... > > > > I'll put this in already: > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > But probably want this also checked by either or both of Laurent and > > Paul who know the IPC mechanisms well. > > > >>> But maybe its something we want to be able to profile/monitor in the > >>> future. > >>> > >>> Or perhaps can we find a way to align data when it goes into the > >>> ByteStreamBuffer with some alignment padding sections somehow? > >> > >> I could be missing something, but given how ipc serialization is done > >> at the moment, it does not seem entirely trivial. What's our alignment issue here, what field(s) are unaligned ? The header contains 32-bit fields, are we serializing data in such a way that causes ipa_controls_header instances to not have a 4 bytes alignment (as in hdr.size not being a multiple of 4) ? Or is the buffer itself not aligned ? > >>> If fixing/specifying the alignment in the buffer isn't possible easy - > >>> then I could certainly imagine accepting this as a current possible solution ... > >>> > >>>> LOG(Serializer, Error) << "Out of data"; > >>>> return {}; > >>>> } > >>>> > >>>> - auto iter = infoMaps_.find(hdr->handle); > >>>> + auto iter = infoMaps_.find(hdr.handle); > >>>> if (iter != infoMaps_.end()) { > >>>> LOG(Serializer, Debug) << "Use cached ControlInfoMap"; > >>>> return iter->second; > >>>> } > >>>> > >>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { > >>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { > >>>> LOG(Serializer, Error) > >>>> << "Unsupported controls format version " > >>>> - << hdr->version; > >>>> + << hdr.version; > >>>> return {}; > >>>> } > >>>> > >>>> @@ -455,7 +455,7 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > >>>> */ > >>>> const ControlIdMap *idMap = nullptr; > >>>> ControlIdMap *localIdMap = nullptr; > >>>> - switch (hdr->id_map_type) { > >>>> + switch (hdr.id_map_type) { > >>>> case IPA_CONTROL_ID_MAP_CONTROLS: > >>>> idMap = &controls::controls; > >>>> break; > >>>> @@ -469,12 +469,12 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > >>>> break; > >>>> default: > >>>> LOG(Serializer, Error) > >>>> - << "Unknown id map type: " << hdr->id_map_type; > >>>> + << "Unknown id map type: " << hdr.id_map_type; > >>>> return {}; > >>>> } > >>>> > >>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); > >>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); > >>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); > >>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); > >>>> > >>>> if (buffer.overflow()) { > >>>> LOG(Serializer, Error) << "Out of data"; > >>>> @@ -482,36 +482,35 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > >>>> } > >>>> > >>>> ControlInfoMap::Map ctrls; > >>>> - for (unsigned int i = 0; i < hdr->entries; ++i) { > >>>> - const struct ipa_control_info_entry *entry = > >>>> - entries.read<decltype(*entry)>(); > >>>> - if (!entry) { > >>>> + for (unsigned int i = 0; i < hdr.entries; ++i) { > >>>> + struct ipa_control_info_entry entry; > >>>> + if (entries.read(&entry) < 0) { > >>>> LOG(Serializer, Error) << "Out of data"; > >>>> return {}; > >>>> } > >>>> > >>>> - ControlType type = static_cast<ControlType>(entry->type); > >>>> + ControlType type = static_cast<ControlType>(entry.type); > >>>> > >>>> /* If we're using a local id map, populate it. */ > >>>> if (localIdMap) { > >>>> ControlId::DirectionFlags flags{ > >>>> - static_cast<ControlId::Direction>(entry->direction) > >>>> + static_cast<ControlId::Direction>(entry.direction) > >>>> }; > >>>> > >>>> /** > >>>> * \todo Find a way to preserve the control name for > >>>> * debugging purpose. > >>>> */ > >>>> - controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, > >>>> + controlIds_.emplace_back(std::make_unique<ControlId>(entry.id, > >>>> "", "local", type, > >>>> flags)); > >>>> - (*localIdMap)[entry->id] = controlIds_.back().get(); > >>>> + (*localIdMap)[entry.id] = controlIds_.back().get(); > >>>> } > >>>> > >>>> - const ControlId *controlId = idMap->at(entry->id); > >>>> + const ControlId *controlId = idMap->at(entry.id); > >>>> ASSERT(controlId); > >>>> > >>>> - if (entry->offset != values.offset()) { > >>>> + if (entry.offset != values.offset()) { > >>>> LOG(Serializer, Error) > >>>> << "Bad data, entry offset mismatch (entry " > >>>> << i << ")"; > >>>> @@ -526,9 +525,9 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > >>>> * Create the ControlInfoMap in the cache, and store the map to handle > >>>> * association. > >>>> */ > >>>> - infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap); > >>>> - ControlInfoMap &map = infoMaps_[hdr->handle]; > >>>> - infoMapHandles_[&map] = hdr->handle; > >>>> + infoMaps_[hdr.handle] = ControlInfoMap(std::move(ctrls), *idMap); > >>>> + ControlInfoMap &map = infoMaps_[hdr.handle]; > >>>> + infoMapHandles_[&map] = hdr.handle; > >>>> > >>>> return map; > >>>> } > >>>> @@ -545,21 +544,21 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > >>>> template<> > >>>> ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer) > >>>> { > >>>> - const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); > >>>> - if (!hdr) { > >>>> + struct ipa_controls_header hdr; > >>>> + if (buffer.read(&hdr) < 0) { > >>>> LOG(Serializer, Error) << "Out of data"; > >>>> return {}; > >>>> } > >>>> > >>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { > >>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { > >>>> LOG(Serializer, Error) > >>>> << "Unsupported controls format version " > >>>> - << hdr->version; > >>>> + << hdr.version; > >>>> return {}; > >>>> } > >>>> > >>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); > >>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); > >>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); > >>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); > >>>> > >>>> if (buffer.overflow()) { > >>>> LOG(Serializer, Error) << "Out of data"; > >>>> @@ -576,10 +575,10 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > >>>> * ControlInfoMap is available. > >>>> */ > >>>> const ControlIdMap *idMap; > >>>> - if (hdr->handle) { > >>>> + if (hdr.handle) { > >>>> auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(), > >>>> [&](decltype(infoMapHandles_)::value_type &entry) { > >>>> - return entry.second == hdr->handle; > >>>> + return entry.second == hdr.handle; > >>>> }); > >>>> if (iter == infoMapHandles_.end()) { > >>>> LOG(Serializer, Error) > >>>> @@ -590,7 +589,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > >>>> const ControlInfoMap *infoMap = iter->first; > >>>> idMap = &infoMap->idmap(); > >>>> } else { > >>>> - switch (hdr->id_map_type) { > >>>> + switch (hdr.id_map_type) { > >>>> case IPA_CONTROL_ID_MAP_CONTROLS: > >>>> idMap = &controls::controls; > >>>> break; > >>>> @@ -601,7 +600,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > >>>> > >>>> case IPA_CONTROL_ID_MAP_V4L2: > >>>> default: > >>>> - if (hdr->entries > 0) > >>>> + if (hdr.entries > 0) > >>>> LOG(Serializer, Fatal) > >>>> << "A list of V4L2 controls requires a ControlInfoMap"; > >>>> > >>>> @@ -617,23 +616,21 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > >>>> */ > >>>> ControlList ctrls(*idMap); > >>>> > >>>> - for (unsigned int i = 0; i < hdr->entries; ++i) { > >>>> - const struct ipa_control_value_entry *entry = > >>>> - entries.read<decltype(*entry)>(); > >>>> - if (!entry) { > >>>> + for (unsigned int i = 0; i < hdr.entries; ++i) { > >>>> + struct ipa_control_value_entry entry; > >>>> + if (entries.read(&entry) < 0) { > >>>> LOG(Serializer, Error) << "Out of data"; > >>>> return {}; > >>>> } > >>>> > >>>> - if (entry->offset != values.offset()) { > >>>> + if (entry.offset != values.offset()) { > >>>> LOG(Serializer, Error) > >>>> << "Bad data, entry offset mismatch (entry " > >>>> << i << ")"; > >>>> return {}; > >>>> } > >>>> > >>>> - ctrls.set(entry->id, > >>>> - loadControlValue(values, entry->is_array, entry->count)); > >>>> + ctrls.set(entry.id, loadControlValue(values, entry.is_array, entry.count)); > >>>> } > >>>> > >>>> return ctrls;
2025. 08. 11. 16:38 keltezéssel, Laurent Pinchart írta: > On Mon, Aug 11, 2025 at 04:28:10PM +0200, Barnabás Pőcze wrote: >> 2025. 08. 11. 16:23 keltezéssel, Kieran Bingham írta: >>> Quoting Barnabás Pőcze (2025-08-11 15:18:57) >>>> 2025. 08. 11. 16:12 keltezéssel, Kieran Bingham írta: >>>>> Quoting Barnabás Pőcze (2025-08-11 14:11:59) >>>>>> Aliasing an arbitrary buffer with some `T *` is unlikely to yield good >>>>>> results, primarily due to potential alignment issues. So don't do it, >>>>>> instead create the objects on the stack and read into them, this >>>>>> removes any and all strict aliasing or alignment concerns. >>>>>> >>>>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=221 >>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>>>> --- >>>>>> src/libcamera/control_serializer.cpp | 75 +++++++++++++--------------- >>>>>> 1 file changed, 36 insertions(+), 39 deletions(-) >>>>>> >>>>>> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp >>>>>> index 050f8512b..722ff231c 100644 >>>>>> --- a/src/libcamera/control_serializer.cpp >>>>>> +++ b/src/libcamera/control_serializer.cpp >>>>>> @@ -427,22 +427,22 @@ ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b) >>>>>> template<> >>>>>> ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &buffer) >>>>>> { >>>>>> - const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); >>>>>> - if (!hdr) { >>>>>> + struct ipa_controls_header hdr; >>>>>> + if (buffer.read(&hdr) < 0) { >>>>> >>>>> Aha, but the key difference is that before it was 'zero copy' just >>>>> pointing at the data, while now we're making a copy ? >>>> >>>> Yes. >>>> >>>>> I wonder if we can detect the alignment and only do the copy if >>>>> unaligned? Or maybe that's just not worth it and we should just accept >>>>> the copy - this shouldn't be a high data volume for the most parts... >>>> >>>> I feel like the control id/info map/list/value allocation easily dominates the >>>> cost of these small copies. These are all <=32 bytes. >>> >>> Hrm.. indeed - we don't normally expect ControlLists to contain large >>> data. >>> >>> I thnik CCMs with a 3x3 matrix is 'hopefully' the biggest we expect for >>> the most part and things like lens shading tables shouldn't be >>> popultated through a control list. >> >> This change only affects the headers, e.g. the bytes of the control value >> data is still read the same way, directly from the buffer. >> >>> So ... yes a copy seems quick and trivial... >>> >>> I'll put this in already: >>> >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> >>> But probably want this also checked by either or both of Laurent and >>> Paul who know the IPC mechanisms well. >>> >>>>> But maybe its something we want to be able to profile/monitor in the >>>>> future. >>>>> >>>>> Or perhaps can we find a way to align data when it goes into the >>>>> ByteStreamBuffer with some alignment padding sections somehow? >>>> >>>> I could be missing something, but given how ipc serialization is done >>>> at the moment, it does not seem entirely trivial. > > What's our alignment issue here, what field(s) are unaligned ? The > header contains 32-bit fields, are we serializing data in such a way > that causes ipa_controls_header instances to not have a 4 bytes > alignment (as in hdr.size not being a multiple of 4) ? Or is the buffer > itself not aligned ? I will turn the question around: what would guarantee the alignment? As far as I can tell, there is nothing. `ControlSerializer` does not guarantee any alignment in and of itself (e.g. `ipa_controls_header::size` can easily be not divisible by 4, e.g. `ControlSerializer::binarySize()` of a uint32_t is 5). And even if it did guarantee alignment, an appropriately sized string serialized before it can easily throw the final alignment off. > >>>>> If fixing/specifying the alignment in the buffer isn't possible easy - >>>>> then I could certainly imagine accepting this as a current possible solution ... >>>>> >>>>>> LOG(Serializer, Error) << "Out of data"; >>>>>> return {}; >>>>>> } >>>>>> >>>>>> - auto iter = infoMaps_.find(hdr->handle); >>>>>> + auto iter = infoMaps_.find(hdr.handle); >>>>>> if (iter != infoMaps_.end()) { >>>>>> LOG(Serializer, Debug) << "Use cached ControlInfoMap"; >>>>>> return iter->second; >>>>>> } >>>>>> >>>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { >>>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { >>>>>> LOG(Serializer, Error) >>>>>> << "Unsupported controls format version " >>>>>> - << hdr->version; >>>>>> + << hdr.version; >>>>>> return {}; >>>>>> } >>>>>> >>>>>> @@ -455,7 +455,7 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & >>>>>> */ >>>>>> const ControlIdMap *idMap = nullptr; >>>>>> ControlIdMap *localIdMap = nullptr; >>>>>> - switch (hdr->id_map_type) { >>>>>> + switch (hdr.id_map_type) { >>>>>> case IPA_CONTROL_ID_MAP_CONTROLS: >>>>>> idMap = &controls::controls; >>>>>> break; >>>>>> @@ -469,12 +469,12 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & >>>>>> break; >>>>>> default: >>>>>> LOG(Serializer, Error) >>>>>> - << "Unknown id map type: " << hdr->id_map_type; >>>>>> + << "Unknown id map type: " << hdr.id_map_type; >>>>>> return {}; >>>>>> } >>>>>> >>>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); >>>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); >>>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); >>>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); >>>>>> >>>>>> if (buffer.overflow()) { >>>>>> LOG(Serializer, Error) << "Out of data"; >>>>>> @@ -482,36 +482,35 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & >>>>>> } >>>>>> >>>>>> ControlInfoMap::Map ctrls; >>>>>> - for (unsigned int i = 0; i < hdr->entries; ++i) { >>>>>> - const struct ipa_control_info_entry *entry = >>>>>> - entries.read<decltype(*entry)>(); >>>>>> - if (!entry) { >>>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) { >>>>>> + struct ipa_control_info_entry entry; >>>>>> + if (entries.read(&entry) < 0) { >>>>>> LOG(Serializer, Error) << "Out of data"; >>>>>> return {}; >>>>>> } >>>>>> >>>>>> - ControlType type = static_cast<ControlType>(entry->type); >>>>>> + ControlType type = static_cast<ControlType>(entry.type); >>>>>> >>>>>> /* If we're using a local id map, populate it. */ >>>>>> if (localIdMap) { >>>>>> ControlId::DirectionFlags flags{ >>>>>> - static_cast<ControlId::Direction>(entry->direction) >>>>>> + static_cast<ControlId::Direction>(entry.direction) >>>>>> }; >>>>>> >>>>>> /** >>>>>> * \todo Find a way to preserve the control name for >>>>>> * debugging purpose. >>>>>> */ >>>>>> - controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, >>>>>> + controlIds_.emplace_back(std::make_unique<ControlId>(entry.id, >>>>>> "", "local", type, >>>>>> flags)); >>>>>> - (*localIdMap)[entry->id] = controlIds_.back().get(); >>>>>> + (*localIdMap)[entry.id] = controlIds_.back().get(); >>>>>> } >>>>>> >>>>>> - const ControlId *controlId = idMap->at(entry->id); >>>>>> + const ControlId *controlId = idMap->at(entry.id); >>>>>> ASSERT(controlId); >>>>>> >>>>>> - if (entry->offset != values.offset()) { >>>>>> + if (entry.offset != values.offset()) { >>>>>> LOG(Serializer, Error) >>>>>> << "Bad data, entry offset mismatch (entry " >>>>>> << i << ")"; >>>>>> @@ -526,9 +525,9 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & >>>>>> * Create the ControlInfoMap in the cache, and store the map to handle >>>>>> * association. >>>>>> */ >>>>>> - infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap); >>>>>> - ControlInfoMap &map = infoMaps_[hdr->handle]; >>>>>> - infoMapHandles_[&map] = hdr->handle; >>>>>> + infoMaps_[hdr.handle] = ControlInfoMap(std::move(ctrls), *idMap); >>>>>> + ControlInfoMap &map = infoMaps_[hdr.handle]; >>>>>> + infoMapHandles_[&map] = hdr.handle; >>>>>> >>>>>> return map; >>>>>> } >>>>>> @@ -545,21 +544,21 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & >>>>>> template<> >>>>>> ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer) >>>>>> { >>>>>> - const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); >>>>>> - if (!hdr) { >>>>>> + struct ipa_controls_header hdr; >>>>>> + if (buffer.read(&hdr) < 0) { >>>>>> LOG(Serializer, Error) << "Out of data"; >>>>>> return {}; >>>>>> } >>>>>> >>>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { >>>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { >>>>>> LOG(Serializer, Error) >>>>>> << "Unsupported controls format version " >>>>>> - << hdr->version; >>>>>> + << hdr.version; >>>>>> return {}; >>>>>> } >>>>>> >>>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); >>>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); >>>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); >>>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); >>>>>> >>>>>> if (buffer.overflow()) { >>>>>> LOG(Serializer, Error) << "Out of data"; >>>>>> @@ -576,10 +575,10 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer >>>>>> * ControlInfoMap is available. >>>>>> */ >>>>>> const ControlIdMap *idMap; >>>>>> - if (hdr->handle) { >>>>>> + if (hdr.handle) { >>>>>> auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(), >>>>>> [&](decltype(infoMapHandles_)::value_type &entry) { >>>>>> - return entry.second == hdr->handle; >>>>>> + return entry.second == hdr.handle; >>>>>> }); >>>>>> if (iter == infoMapHandles_.end()) { >>>>>> LOG(Serializer, Error) >>>>>> @@ -590,7 +589,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer >>>>>> const ControlInfoMap *infoMap = iter->first; >>>>>> idMap = &infoMap->idmap(); >>>>>> } else { >>>>>> - switch (hdr->id_map_type) { >>>>>> + switch (hdr.id_map_type) { >>>>>> case IPA_CONTROL_ID_MAP_CONTROLS: >>>>>> idMap = &controls::controls; >>>>>> break; >>>>>> @@ -601,7 +600,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer >>>>>> >>>>>> case IPA_CONTROL_ID_MAP_V4L2: >>>>>> default: >>>>>> - if (hdr->entries > 0) >>>>>> + if (hdr.entries > 0) >>>>>> LOG(Serializer, Fatal) >>>>>> << "A list of V4L2 controls requires a ControlInfoMap"; >>>>>> >>>>>> @@ -617,23 +616,21 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer >>>>>> */ >>>>>> ControlList ctrls(*idMap); >>>>>> >>>>>> - for (unsigned int i = 0; i < hdr->entries; ++i) { >>>>>> - const struct ipa_control_value_entry *entry = >>>>>> - entries.read<decltype(*entry)>(); >>>>>> - if (!entry) { >>>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) { >>>>>> + struct ipa_control_value_entry entry; >>>>>> + if (entries.read(&entry) < 0) { >>>>>> LOG(Serializer, Error) << "Out of data"; >>>>>> return {}; >>>>>> } >>>>>> >>>>>> - if (entry->offset != values.offset()) { >>>>>> + if (entry.offset != values.offset()) { >>>>>> LOG(Serializer, Error) >>>>>> << "Bad data, entry offset mismatch (entry " >>>>>> << i << ")"; >>>>>> return {}; >>>>>> } >>>>>> >>>>>> - ctrls.set(entry->id, >>>>>> - loadControlValue(values, entry->is_array, entry->count)); >>>>>> + ctrls.set(entry.id, loadControlValue(values, entry.is_array, entry.count)); >>>>>> } >>>>>> >>>>>> return ctrls; >
On Mon, Aug 11, 2025 at 04:52:45PM +0200, Barnabás Pőcze wrote: > 2025. 08. 11. 16:38 keltezéssel, Laurent Pinchart írta: > > On Mon, Aug 11, 2025 at 04:28:10PM +0200, Barnabás Pőcze wrote: > >> 2025. 08. 11. 16:23 keltezéssel, Kieran Bingham írta: > >>> Quoting Barnabás Pőcze (2025-08-11 15:18:57) > >>>> 2025. 08. 11. 16:12 keltezéssel, Kieran Bingham írta: > >>>>> Quoting Barnabás Pőcze (2025-08-11 14:11:59) > >>>>>> Aliasing an arbitrary buffer with some `T *` is unlikely to yield good > >>>>>> results, primarily due to potential alignment issues. So don't do it, > >>>>>> instead create the objects on the stack and read into them, this > >>>>>> removes any and all strict aliasing or alignment concerns. > >>>>>> > >>>>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=221 > >>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >>>>>> --- > >>>>>> src/libcamera/control_serializer.cpp | 75 +++++++++++++--------------- > >>>>>> 1 file changed, 36 insertions(+), 39 deletions(-) > >>>>>> > >>>>>> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > >>>>>> index 050f8512b..722ff231c 100644 > >>>>>> --- a/src/libcamera/control_serializer.cpp > >>>>>> +++ b/src/libcamera/control_serializer.cpp > >>>>>> @@ -427,22 +427,22 @@ ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b) > >>>>>> template<> > >>>>>> ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &buffer) > >>>>>> { > >>>>>> - const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); > >>>>>> - if (!hdr) { > >>>>>> + struct ipa_controls_header hdr; > >>>>>> + if (buffer.read(&hdr) < 0) { > >>>>> > >>>>> Aha, but the key difference is that before it was 'zero copy' just > >>>>> pointing at the data, while now we're making a copy ? > >>>> > >>>> Yes. > >>>> > >>>>> I wonder if we can detect the alignment and only do the copy if > >>>>> unaligned? Or maybe that's just not worth it and we should just accept > >>>>> the copy - this shouldn't be a high data volume for the most parts... > >>>> > >>>> I feel like the control id/info map/list/value allocation easily dominates the > >>>> cost of these small copies. These are all <=32 bytes. > >>> > >>> Hrm.. indeed - we don't normally expect ControlLists to contain large > >>> data. > >>> > >>> I thnik CCMs with a 3x3 matrix is 'hopefully' the biggest we expect for > >>> the most part and things like lens shading tables shouldn't be > >>> popultated through a control list. > >> > >> This change only affects the headers, e.g. the bytes of the control value > >> data is still read the same way, directly from the buffer. > >> > >>> So ... yes a copy seems quick and trivial... > >>> > >>> I'll put this in already: > >>> > >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>> > >>> But probably want this also checked by either or both of Laurent and > >>> Paul who know the IPC mechanisms well. > >>> > >>>>> But maybe its something we want to be able to profile/monitor in the > >>>>> future. > >>>>> > >>>>> Or perhaps can we find a way to align data when it goes into the > >>>>> ByteStreamBuffer with some alignment padding sections somehow? > >>>> > >>>> I could be missing something, but given how ipc serialization is done > >>>> at the moment, it does not seem entirely trivial. > > > > What's our alignment issue here, what field(s) are unaligned ? The > > header contains 32-bit fields, are we serializing data in such a way > > that causes ipa_controls_header instances to not have a 4 bytes > > alignment (as in hdr.size not being a multiple of 4) ? Or is the buffer > > itself not aligned ? > > I will turn the question around: what would guarantee the alignment? > As far as I can tell, there is nothing. `ControlSerializer` does not guarantee > any alignment in and of itself (e.g. `ipa_controls_header::size` can easily be > not divisible by 4, e.g. `ControlSerializer::binarySize()` of a uint32_t is 5). > And even if it did guarantee alignment, an appropriately sized string serialized > before it can easily throw the final alignment off. I thought we were supposed to pad control packets to multiples of 8 bytes, as described in ipa_controls.cpp. That's what surprises me here. > >>>>> If fixing/specifying the alignment in the buffer isn't possible easy - > >>>>> then I could certainly imagine accepting this as a current possible solution ... > >>>>> > >>>>>> LOG(Serializer, Error) << "Out of data"; > >>>>>> return {}; > >>>>>> } > >>>>>> > >>>>>> - auto iter = infoMaps_.find(hdr->handle); > >>>>>> + auto iter = infoMaps_.find(hdr.handle); > >>>>>> if (iter != infoMaps_.end()) { > >>>>>> LOG(Serializer, Debug) << "Use cached ControlInfoMap"; > >>>>>> return iter->second; > >>>>>> } > >>>>>> > >>>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { > >>>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { > >>>>>> LOG(Serializer, Error) > >>>>>> << "Unsupported controls format version " > >>>>>> - << hdr->version; > >>>>>> + << hdr.version; > >>>>>> return {}; > >>>>>> } > >>>>>> > >>>>>> @@ -455,7 +455,7 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > >>>>>> */ > >>>>>> const ControlIdMap *idMap = nullptr; > >>>>>> ControlIdMap *localIdMap = nullptr; > >>>>>> - switch (hdr->id_map_type) { > >>>>>> + switch (hdr.id_map_type) { > >>>>>> case IPA_CONTROL_ID_MAP_CONTROLS: > >>>>>> idMap = &controls::controls; > >>>>>> break; > >>>>>> @@ -469,12 +469,12 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > >>>>>> break; > >>>>>> default: > >>>>>> LOG(Serializer, Error) > >>>>>> - << "Unknown id map type: " << hdr->id_map_type; > >>>>>> + << "Unknown id map type: " << hdr.id_map_type; > >>>>>> return {}; > >>>>>> } > >>>>>> > >>>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); > >>>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); > >>>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); > >>>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); > >>>>>> > >>>>>> if (buffer.overflow()) { > >>>>>> LOG(Serializer, Error) << "Out of data"; > >>>>>> @@ -482,36 +482,35 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > >>>>>> } > >>>>>> > >>>>>> ControlInfoMap::Map ctrls; > >>>>>> - for (unsigned int i = 0; i < hdr->entries; ++i) { > >>>>>> - const struct ipa_control_info_entry *entry = > >>>>>> - entries.read<decltype(*entry)>(); > >>>>>> - if (!entry) { > >>>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) { > >>>>>> + struct ipa_control_info_entry entry; > >>>>>> + if (entries.read(&entry) < 0) { > >>>>>> LOG(Serializer, Error) << "Out of data"; > >>>>>> return {}; > >>>>>> } > >>>>>> > >>>>>> - ControlType type = static_cast<ControlType>(entry->type); > >>>>>> + ControlType type = static_cast<ControlType>(entry.type); > >>>>>> > >>>>>> /* If we're using a local id map, populate it. */ > >>>>>> if (localIdMap) { > >>>>>> ControlId::DirectionFlags flags{ > >>>>>> - static_cast<ControlId::Direction>(entry->direction) > >>>>>> + static_cast<ControlId::Direction>(entry.direction) > >>>>>> }; > >>>>>> > >>>>>> /** > >>>>>> * \todo Find a way to preserve the control name for > >>>>>> * debugging purpose. > >>>>>> */ > >>>>>> - controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, > >>>>>> + controlIds_.emplace_back(std::make_unique<ControlId>(entry.id, > >>>>>> "", "local", type, > >>>>>> flags)); > >>>>>> - (*localIdMap)[entry->id] = controlIds_.back().get(); > >>>>>> + (*localIdMap)[entry.id] = controlIds_.back().get(); > >>>>>> } > >>>>>> > >>>>>> - const ControlId *controlId = idMap->at(entry->id); > >>>>>> + const ControlId *controlId = idMap->at(entry.id); > >>>>>> ASSERT(controlId); > >>>>>> > >>>>>> - if (entry->offset != values.offset()) { > >>>>>> + if (entry.offset != values.offset()) { > >>>>>> LOG(Serializer, Error) > >>>>>> << "Bad data, entry offset mismatch (entry " > >>>>>> << i << ")"; > >>>>>> @@ -526,9 +525,9 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > >>>>>> * Create the ControlInfoMap in the cache, and store the map to handle > >>>>>> * association. > >>>>>> */ > >>>>>> - infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap); > >>>>>> - ControlInfoMap &map = infoMaps_[hdr->handle]; > >>>>>> - infoMapHandles_[&map] = hdr->handle; > >>>>>> + infoMaps_[hdr.handle] = ControlInfoMap(std::move(ctrls), *idMap); > >>>>>> + ControlInfoMap &map = infoMaps_[hdr.handle]; > >>>>>> + infoMapHandles_[&map] = hdr.handle; > >>>>>> > >>>>>> return map; > >>>>>> } > >>>>>> @@ -545,21 +544,21 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > >>>>>> template<> > >>>>>> ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer) > >>>>>> { > >>>>>> - const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); > >>>>>> - if (!hdr) { > >>>>>> + struct ipa_controls_header hdr; > >>>>>> + if (buffer.read(&hdr) < 0) { > >>>>>> LOG(Serializer, Error) << "Out of data"; > >>>>>> return {}; > >>>>>> } > >>>>>> > >>>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { > >>>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { > >>>>>> LOG(Serializer, Error) > >>>>>> << "Unsupported controls format version " > >>>>>> - << hdr->version; > >>>>>> + << hdr.version; > >>>>>> return {}; > >>>>>> } > >>>>>> > >>>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); > >>>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); > >>>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); > >>>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); > >>>>>> > >>>>>> if (buffer.overflow()) { > >>>>>> LOG(Serializer, Error) << "Out of data"; > >>>>>> @@ -576,10 +575,10 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > >>>>>> * ControlInfoMap is available. > >>>>>> */ > >>>>>> const ControlIdMap *idMap; > >>>>>> - if (hdr->handle) { > >>>>>> + if (hdr.handle) { > >>>>>> auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(), > >>>>>> [&](decltype(infoMapHandles_)::value_type &entry) { > >>>>>> - return entry.second == hdr->handle; > >>>>>> + return entry.second == hdr.handle; > >>>>>> }); > >>>>>> if (iter == infoMapHandles_.end()) { > >>>>>> LOG(Serializer, Error) > >>>>>> @@ -590,7 +589,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > >>>>>> const ControlInfoMap *infoMap = iter->first; > >>>>>> idMap = &infoMap->idmap(); > >>>>>> } else { > >>>>>> - switch (hdr->id_map_type) { > >>>>>> + switch (hdr.id_map_type) { > >>>>>> case IPA_CONTROL_ID_MAP_CONTROLS: > >>>>>> idMap = &controls::controls; > >>>>>> break; > >>>>>> @@ -601,7 +600,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > >>>>>> > >>>>>> case IPA_CONTROL_ID_MAP_V4L2: > >>>>>> default: > >>>>>> - if (hdr->entries > 0) > >>>>>> + if (hdr.entries > 0) > >>>>>> LOG(Serializer, Fatal) > >>>>>> << "A list of V4L2 controls requires a ControlInfoMap"; > >>>>>> > >>>>>> @@ -617,23 +616,21 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > >>>>>> */ > >>>>>> ControlList ctrls(*idMap); > >>>>>> > >>>>>> - for (unsigned int i = 0; i < hdr->entries; ++i) { > >>>>>> - const struct ipa_control_value_entry *entry = > >>>>>> - entries.read<decltype(*entry)>(); > >>>>>> - if (!entry) { > >>>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) { > >>>>>> + struct ipa_control_value_entry entry; > >>>>>> + if (entries.read(&entry) < 0) { > >>>>>> LOG(Serializer, Error) << "Out of data"; > >>>>>> return {}; > >>>>>> } > >>>>>> > >>>>>> - if (entry->offset != values.offset()) { > >>>>>> + if (entry.offset != values.offset()) { > >>>>>> LOG(Serializer, Error) > >>>>>> << "Bad data, entry offset mismatch (entry " > >>>>>> << i << ")"; > >>>>>> return {}; > >>>>>> } > >>>>>> > >>>>>> - ctrls.set(entry->id, > >>>>>> - loadControlValue(values, entry->is_array, entry->count)); > >>>>>> + ctrls.set(entry.id, loadControlValue(values, entry.is_array, entry.count)); > >>>>>> } > >>>>>> > >>>>>> return ctrls; > > >
2025. 08. 11. 17:21 keltezéssel, Laurent Pinchart írta: > On Mon, Aug 11, 2025 at 04:52:45PM +0200, Barnabás Pőcze wrote: >> 2025. 08. 11. 16:38 keltezéssel, Laurent Pinchart írta: >>> On Mon, Aug 11, 2025 at 04:28:10PM +0200, Barnabás Pőcze wrote: >>>> 2025. 08. 11. 16:23 keltezéssel, Kieran Bingham írta: >>>>> Quoting Barnabás Pőcze (2025-08-11 15:18:57) >>>>>> 2025. 08. 11. 16:12 keltezéssel, Kieran Bingham írta: >>>>>>> Quoting Barnabás Pőcze (2025-08-11 14:11:59) >>>>>>>> Aliasing an arbitrary buffer with some `T *` is unlikely to yield good >>>>>>>> results, primarily due to potential alignment issues. So don't do it, >>>>>>>> instead create the objects on the stack and read into them, this >>>>>>>> removes any and all strict aliasing or alignment concerns. >>>>>>>> >>>>>>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=221 >>>>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>>>>>> --- >>>>>>>> src/libcamera/control_serializer.cpp | 75 +++++++++++++--------------- >>>>>>>> 1 file changed, 36 insertions(+), 39 deletions(-) >>>>>>>> >>>>>>>> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp >>>>>>>> index 050f8512b..722ff231c 100644 >>>>>>>> --- a/src/libcamera/control_serializer.cpp >>>>>>>> +++ b/src/libcamera/control_serializer.cpp >>>>>>>> @@ -427,22 +427,22 @@ ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b) >>>>>>>> template<> >>>>>>>> ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &buffer) >>>>>>>> { >>>>>>>> - const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); >>>>>>>> - if (!hdr) { >>>>>>>> + struct ipa_controls_header hdr; >>>>>>>> + if (buffer.read(&hdr) < 0) { >>>>>>> >>>>>>> Aha, but the key difference is that before it was 'zero copy' just >>>>>>> pointing at the data, while now we're making a copy ? >>>>>> >>>>>> Yes. >>>>>> >>>>>>> I wonder if we can detect the alignment and only do the copy if >>>>>>> unaligned? Or maybe that's just not worth it and we should just accept >>>>>>> the copy - this shouldn't be a high data volume for the most parts... >>>>>> >>>>>> I feel like the control id/info map/list/value allocation easily dominates the >>>>>> cost of these small copies. These are all <=32 bytes. >>>>> >>>>> Hrm.. indeed - we don't normally expect ControlLists to contain large >>>>> data. >>>>> >>>>> I thnik CCMs with a 3x3 matrix is 'hopefully' the biggest we expect for >>>>> the most part and things like lens shading tables shouldn't be >>>>> popultated through a control list. >>>> >>>> This change only affects the headers, e.g. the bytes of the control value >>>> data is still read the same way, directly from the buffer. >>>> >>>>> So ... yes a copy seems quick and trivial... >>>>> >>>>> I'll put this in already: >>>>> >>>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>>> >>>>> But probably want this also checked by either or both of Laurent and >>>>> Paul who know the IPC mechanisms well. >>>>> >>>>>>> But maybe its something we want to be able to profile/monitor in the >>>>>>> future. >>>>>>> >>>>>>> Or perhaps can we find a way to align data when it goes into the >>>>>>> ByteStreamBuffer with some alignment padding sections somehow? >>>>>> >>>>>> I could be missing something, but given how ipc serialization is done >>>>>> at the moment, it does not seem entirely trivial. >>> >>> What's our alignment issue here, what field(s) are unaligned ? The >>> header contains 32-bit fields, are we serializing data in such a way >>> that causes ipa_controls_header instances to not have a 4 bytes >>> alignment (as in hdr.size not being a multiple of 4) ? Or is the buffer >>> itself not aligned ? >> >> I will turn the question around: what would guarantee the alignment? >> As far as I can tell, there is nothing. `ControlSerializer` does not guarantee >> any alignment in and of itself (e.g. `ipa_controls_header::size` can easily be >> not divisible by 4, e.g. `ControlSerializer::binarySize()` of a uint32_t is 5). >> And even if it did guarantee alignment, an appropriately sized string serialized >> before it can easily throw the final alignment off. > > I thought we were supposed to pad control packets to multiples of 8 > bytes, as described in ipa_controls.cpp. That's what surprises me here. Ahh, that's true. I haven't noticed that. How should we proceed then? I would like to go ahead with this change now for two reasons: * the way serialization is done does not seem to guarantee sufficient alignment even if the control lists, etc. were serialized respecting the alignment requirements; * I would like to incorporate https://patchwork.libcamera.org/patch/23373/ which changes at the least code appreciably, so I think it would be better to return to the question of alignment after that. Regards, Barnabás Pőcze > >>>>>>> If fixing/specifying the alignment in the buffer isn't possible easy - >>>>>>> then I could certainly imagine accepting this as a current possible solution ... >>>>>>> >>>>>>>> LOG(Serializer, Error) << "Out of data"; >>>>>>>> return {}; >>>>>>>> } >>>>>>>> >>>>>>>> - auto iter = infoMaps_.find(hdr->handle); >>>>>>>> + auto iter = infoMaps_.find(hdr.handle); >>>>>>>> if (iter != infoMaps_.end()) { >>>>>>>> LOG(Serializer, Debug) << "Use cached ControlInfoMap"; >>>>>>>> return iter->second; >>>>>>>> } >>>>>>>> >>>>>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { >>>>>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { >>>>>>>> LOG(Serializer, Error) >>>>>>>> << "Unsupported controls format version " >>>>>>>> - << hdr->version; >>>>>>>> + << hdr.version; >>>>>>>> return {}; >>>>>>>> } >>>>>>>> >>>>>>>> @@ -455,7 +455,7 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & >>>>>>>> */ >>>>>>>> const ControlIdMap *idMap = nullptr; >>>>>>>> ControlIdMap *localIdMap = nullptr; >>>>>>>> - switch (hdr->id_map_type) { >>>>>>>> + switch (hdr.id_map_type) { >>>>>>>> case IPA_CONTROL_ID_MAP_CONTROLS: >>>>>>>> idMap = &controls::controls; >>>>>>>> break; >>>>>>>> @@ -469,12 +469,12 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & >>>>>>>> break; >>>>>>>> default: >>>>>>>> LOG(Serializer, Error) >>>>>>>> - << "Unknown id map type: " << hdr->id_map_type; >>>>>>>> + << "Unknown id map type: " << hdr.id_map_type; >>>>>>>> return {}; >>>>>>>> } >>>>>>>> >>>>>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); >>>>>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); >>>>>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); >>>>>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); >>>>>>>> >>>>>>>> if (buffer.overflow()) { >>>>>>>> LOG(Serializer, Error) << "Out of data"; >>>>>>>> @@ -482,36 +482,35 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & >>>>>>>> } >>>>>>>> >>>>>>>> ControlInfoMap::Map ctrls; >>>>>>>> - for (unsigned int i = 0; i < hdr->entries; ++i) { >>>>>>>> - const struct ipa_control_info_entry *entry = >>>>>>>> - entries.read<decltype(*entry)>(); >>>>>>>> - if (!entry) { >>>>>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) { >>>>>>>> + struct ipa_control_info_entry entry; >>>>>>>> + if (entries.read(&entry) < 0) { >>>>>>>> LOG(Serializer, Error) << "Out of data"; >>>>>>>> return {}; >>>>>>>> } >>>>>>>> >>>>>>>> - ControlType type = static_cast<ControlType>(entry->type); >>>>>>>> + ControlType type = static_cast<ControlType>(entry.type); >>>>>>>> >>>>>>>> /* If we're using a local id map, populate it. */ >>>>>>>> if (localIdMap) { >>>>>>>> ControlId::DirectionFlags flags{ >>>>>>>> - static_cast<ControlId::Direction>(entry->direction) >>>>>>>> + static_cast<ControlId::Direction>(entry.direction) >>>>>>>> }; >>>>>>>> >>>>>>>> /** >>>>>>>> * \todo Find a way to preserve the control name for >>>>>>>> * debugging purpose. >>>>>>>> */ >>>>>>>> - controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, >>>>>>>> + controlIds_.emplace_back(std::make_unique<ControlId>(entry.id, >>>>>>>> "", "local", type, >>>>>>>> flags)); >>>>>>>> - (*localIdMap)[entry->id] = controlIds_.back().get(); >>>>>>>> + (*localIdMap)[entry.id] = controlIds_.back().get(); >>>>>>>> } >>>>>>>> >>>>>>>> - const ControlId *controlId = idMap->at(entry->id); >>>>>>>> + const ControlId *controlId = idMap->at(entry.id); >>>>>>>> ASSERT(controlId); >>>>>>>> >>>>>>>> - if (entry->offset != values.offset()) { >>>>>>>> + if (entry.offset != values.offset()) { >>>>>>>> LOG(Serializer, Error) >>>>>>>> << "Bad data, entry offset mismatch (entry " >>>>>>>> << i << ")"; >>>>>>>> @@ -526,9 +525,9 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & >>>>>>>> * Create the ControlInfoMap in the cache, and store the map to handle >>>>>>>> * association. >>>>>>>> */ >>>>>>>> - infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap); >>>>>>>> - ControlInfoMap &map = infoMaps_[hdr->handle]; >>>>>>>> - infoMapHandles_[&map] = hdr->handle; >>>>>>>> + infoMaps_[hdr.handle] = ControlInfoMap(std::move(ctrls), *idMap); >>>>>>>> + ControlInfoMap &map = infoMaps_[hdr.handle]; >>>>>>>> + infoMapHandles_[&map] = hdr.handle; >>>>>>>> >>>>>>>> return map; >>>>>>>> } >>>>>>>> @@ -545,21 +544,21 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & >>>>>>>> template<> >>>>>>>> ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer) >>>>>>>> { >>>>>>>> - const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); >>>>>>>> - if (!hdr) { >>>>>>>> + struct ipa_controls_header hdr; >>>>>>>> + if (buffer.read(&hdr) < 0) { >>>>>>>> LOG(Serializer, Error) << "Out of data"; >>>>>>>> return {}; >>>>>>>> } >>>>>>>> >>>>>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { >>>>>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { >>>>>>>> LOG(Serializer, Error) >>>>>>>> << "Unsupported controls format version " >>>>>>>> - << hdr->version; >>>>>>>> + << hdr.version; >>>>>>>> return {}; >>>>>>>> } >>>>>>>> >>>>>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); >>>>>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); >>>>>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); >>>>>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); >>>>>>>> >>>>>>>> if (buffer.overflow()) { >>>>>>>> LOG(Serializer, Error) << "Out of data"; >>>>>>>> @@ -576,10 +575,10 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer >>>>>>>> * ControlInfoMap is available. >>>>>>>> */ >>>>>>>> const ControlIdMap *idMap; >>>>>>>> - if (hdr->handle) { >>>>>>>> + if (hdr.handle) { >>>>>>>> auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(), >>>>>>>> [&](decltype(infoMapHandles_)::value_type &entry) { >>>>>>>> - return entry.second == hdr->handle; >>>>>>>> + return entry.second == hdr.handle; >>>>>>>> }); >>>>>>>> if (iter == infoMapHandles_.end()) { >>>>>>>> LOG(Serializer, Error) >>>>>>>> @@ -590,7 +589,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer >>>>>>>> const ControlInfoMap *infoMap = iter->first; >>>>>>>> idMap = &infoMap->idmap(); >>>>>>>> } else { >>>>>>>> - switch (hdr->id_map_type) { >>>>>>>> + switch (hdr.id_map_type) { >>>>>>>> case IPA_CONTROL_ID_MAP_CONTROLS: >>>>>>>> idMap = &controls::controls; >>>>>>>> break; >>>>>>>> @@ -601,7 +600,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer >>>>>>>> >>>>>>>> case IPA_CONTROL_ID_MAP_V4L2: >>>>>>>> default: >>>>>>>> - if (hdr->entries > 0) >>>>>>>> + if (hdr.entries > 0) >>>>>>>> LOG(Serializer, Fatal) >>>>>>>> << "A list of V4L2 controls requires a ControlInfoMap"; >>>>>>>> >>>>>>>> @@ -617,23 +616,21 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer >>>>>>>> */ >>>>>>>> ControlList ctrls(*idMap); >>>>>>>> >>>>>>>> - for (unsigned int i = 0; i < hdr->entries; ++i) { >>>>>>>> - const struct ipa_control_value_entry *entry = >>>>>>>> - entries.read<decltype(*entry)>(); >>>>>>>> - if (!entry) { >>>>>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) { >>>>>>>> + struct ipa_control_value_entry entry; >>>>>>>> + if (entries.read(&entry) < 0) { >>>>>>>> LOG(Serializer, Error) << "Out of data"; >>>>>>>> return {}; >>>>>>>> } >>>>>>>> >>>>>>>> - if (entry->offset != values.offset()) { >>>>>>>> + if (entry.offset != values.offset()) { >>>>>>>> LOG(Serializer, Error) >>>>>>>> << "Bad data, entry offset mismatch (entry " >>>>>>>> << i << ")"; >>>>>>>> return {}; >>>>>>>> } >>>>>>>> >>>>>>>> - ctrls.set(entry->id, >>>>>>>> - loadControlValue(values, entry->is_array, entry->count)); >>>>>>>> + ctrls.set(entry.id, loadControlValue(values, entry.is_array, entry.count)); >>>>>>>> } >>>>>>>> >>>>>>>> return ctrls; >>> >> >
On Wed, Aug 13, 2025 at 12:32:29PM +0200, Barnabás Pőcze wrote: > 2025. 08. 11. 17:21 keltezéssel, Laurent Pinchart írta: > > On Mon, Aug 11, 2025 at 04:52:45PM +0200, Barnabás Pőcze wrote: > >> 2025. 08. 11. 16:38 keltezéssel, Laurent Pinchart írta: > >>> On Mon, Aug 11, 2025 at 04:28:10PM +0200, Barnabás Pőcze wrote: > >>>> 2025. 08. 11. 16:23 keltezéssel, Kieran Bingham írta: > >>>>> Quoting Barnabás Pőcze (2025-08-11 15:18:57) > >>>>>> 2025. 08. 11. 16:12 keltezéssel, Kieran Bingham írta: > >>>>>>> Quoting Barnabás Pőcze (2025-08-11 14:11:59) > >>>>>>>> Aliasing an arbitrary buffer with some `T *` is unlikely to yield good > >>>>>>>> results, primarily due to potential alignment issues. So don't do it, > >>>>>>>> instead create the objects on the stack and read into them, this > >>>>>>>> removes any and all strict aliasing or alignment concerns. > >>>>>>>> > >>>>>>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=221 > >>>>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >>>>>>>> --- > >>>>>>>> src/libcamera/control_serializer.cpp | 75 +++++++++++++--------------- > >>>>>>>> 1 file changed, 36 insertions(+), 39 deletions(-) > >>>>>>>> > >>>>>>>> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > >>>>>>>> index 050f8512b..722ff231c 100644 > >>>>>>>> --- a/src/libcamera/control_serializer.cpp > >>>>>>>> +++ b/src/libcamera/control_serializer.cpp > >>>>>>>> @@ -427,22 +427,22 @@ ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b) > >>>>>>>> template<> > >>>>>>>> ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &buffer) > >>>>>>>> { > >>>>>>>> - const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); > >>>>>>>> - if (!hdr) { > >>>>>>>> + struct ipa_controls_header hdr; > >>>>>>>> + if (buffer.read(&hdr) < 0) { > >>>>>>> > >>>>>>> Aha, but the key difference is that before it was 'zero copy' just > >>>>>>> pointing at the data, while now we're making a copy ? > >>>>>> > >>>>>> Yes. > >>>>>> > >>>>>>> I wonder if we can detect the alignment and only do the copy if > >>>>>>> unaligned? Or maybe that's just not worth it and we should just accept > >>>>>>> the copy - this shouldn't be a high data volume for the most parts... > >>>>>> > >>>>>> I feel like the control id/info map/list/value allocation easily dominates the > >>>>>> cost of these small copies. These are all <=32 bytes. > >>>>> > >>>>> Hrm.. indeed - we don't normally expect ControlLists to contain large > >>>>> data. > >>>>> > >>>>> I thnik CCMs with a 3x3 matrix is 'hopefully' the biggest we expect for > >>>>> the most part and things like lens shading tables shouldn't be > >>>>> popultated through a control list. > >>>> > >>>> This change only affects the headers, e.g. the bytes of the control value > >>>> data is still read the same way, directly from the buffer. > >>>> > >>>>> So ... yes a copy seems quick and trivial... > >>>>> > >>>>> I'll put this in already: > >>>>> > >>>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>>>> > >>>>> But probably want this also checked by either or both of Laurent and > >>>>> Paul who know the IPC mechanisms well. > >>>>> > >>>>>>> But maybe its something we want to be able to profile/monitor in the > >>>>>>> future. > >>>>>>> > >>>>>>> Or perhaps can we find a way to align data when it goes into the > >>>>>>> ByteStreamBuffer with some alignment padding sections somehow? > >>>>>> > >>>>>> I could be missing something, but given how ipc serialization is done > >>>>>> at the moment, it does not seem entirely trivial. > >>> > >>> What's our alignment issue here, what field(s) are unaligned ? The > >>> header contains 32-bit fields, are we serializing data in such a way > >>> that causes ipa_controls_header instances to not have a 4 bytes > >>> alignment (as in hdr.size not being a multiple of 4) ? Or is the buffer > >>> itself not aligned ? > >> > >> I will turn the question around: what would guarantee the alignment? > >> As far as I can tell, there is nothing. `ControlSerializer` does not guarantee > >> any alignment in and of itself (e.g. `ipa_controls_header::size` can easily be > >> not divisible by 4, e.g. `ControlSerializer::binarySize()` of a uint32_t is 5). > >> And even if it did guarantee alignment, an appropriately sized string serialized > >> before it can easily throw the final alignment off. > > > > I thought we were supposed to pad control packets to multiples of 8 > > bytes, as described in ipa_controls.cpp. That's what surprises me here. > > Ahh, that's true. I haven't noticed that. How should we proceed then? I would like to > go ahead with this change now for two reasons: > > * the way serialization is done does not seem to guarantee sufficient alignment even > if the control lists, etc. were serialized respecting the alignment requirements; Does that mean there's a bug in the serialization code that we should fix ? > * I would like to incorporate https://patchwork.libcamera.org/patch/23373/ which > changes at the least code appreciably, so I think it would be better to return > to the question of alignment after that. I agree that this patch will not significantly hinder performance as it only copies the headers. However, with the documented alignment, this copy shouldn't be needed, and avoiding a copy would still be best. Are you proposing merging this patch and https://patchwork.libcamera.org/patch/23373/, and then fix the alignment problem and remove the copy on top ? > >>>>>>> If fixing/specifying the alignment in the buffer isn't possible easy - > >>>>>>> then I could certainly imagine accepting this as a current possible solution ... > >>>>>>> > >>>>>>>> LOG(Serializer, Error) << "Out of data"; > >>>>>>>> return {}; > >>>>>>>> } > >>>>>>>> > >>>>>>>> - auto iter = infoMaps_.find(hdr->handle); > >>>>>>>> + auto iter = infoMaps_.find(hdr.handle); > >>>>>>>> if (iter != infoMaps_.end()) { > >>>>>>>> LOG(Serializer, Debug) << "Use cached ControlInfoMap"; > >>>>>>>> return iter->second; > >>>>>>>> } > >>>>>>>> > >>>>>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { > >>>>>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { > >>>>>>>> LOG(Serializer, Error) > >>>>>>>> << "Unsupported controls format version " > >>>>>>>> - << hdr->version; > >>>>>>>> + << hdr.version; > >>>>>>>> return {}; > >>>>>>>> } > >>>>>>>> > >>>>>>>> @@ -455,7 +455,7 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > >>>>>>>> */ > >>>>>>>> const ControlIdMap *idMap = nullptr; > >>>>>>>> ControlIdMap *localIdMap = nullptr; > >>>>>>>> - switch (hdr->id_map_type) { > >>>>>>>> + switch (hdr.id_map_type) { > >>>>>>>> case IPA_CONTROL_ID_MAP_CONTROLS: > >>>>>>>> idMap = &controls::controls; > >>>>>>>> break; > >>>>>>>> @@ -469,12 +469,12 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > >>>>>>>> break; > >>>>>>>> default: > >>>>>>>> LOG(Serializer, Error) > >>>>>>>> - << "Unknown id map type: " << hdr->id_map_type; > >>>>>>>> + << "Unknown id map type: " << hdr.id_map_type; > >>>>>>>> return {}; > >>>>>>>> } > >>>>>>>> > >>>>>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); > >>>>>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); > >>>>>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); > >>>>>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); > >>>>>>>> > >>>>>>>> if (buffer.overflow()) { > >>>>>>>> LOG(Serializer, Error) << "Out of data"; > >>>>>>>> @@ -482,36 +482,35 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > >>>>>>>> } > >>>>>>>> > >>>>>>>> ControlInfoMap::Map ctrls; > >>>>>>>> - for (unsigned int i = 0; i < hdr->entries; ++i) { > >>>>>>>> - const struct ipa_control_info_entry *entry = > >>>>>>>> - entries.read<decltype(*entry)>(); > >>>>>>>> - if (!entry) { > >>>>>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) { > >>>>>>>> + struct ipa_control_info_entry entry; > >>>>>>>> + if (entries.read(&entry) < 0) { > >>>>>>>> LOG(Serializer, Error) << "Out of data"; > >>>>>>>> return {}; > >>>>>>>> } > >>>>>>>> > >>>>>>>> - ControlType type = static_cast<ControlType>(entry->type); > >>>>>>>> + ControlType type = static_cast<ControlType>(entry.type); > >>>>>>>> > >>>>>>>> /* If we're using a local id map, populate it. */ > >>>>>>>> if (localIdMap) { > >>>>>>>> ControlId::DirectionFlags flags{ > >>>>>>>> - static_cast<ControlId::Direction>(entry->direction) > >>>>>>>> + static_cast<ControlId::Direction>(entry.direction) > >>>>>>>> }; > >>>>>>>> > >>>>>>>> /** > >>>>>>>> * \todo Find a way to preserve the control name for > >>>>>>>> * debugging purpose. > >>>>>>>> */ > >>>>>>>> - controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, > >>>>>>>> + controlIds_.emplace_back(std::make_unique<ControlId>(entry.id, > >>>>>>>> "", "local", type, > >>>>>>>> flags)); > >>>>>>>> - (*localIdMap)[entry->id] = controlIds_.back().get(); > >>>>>>>> + (*localIdMap)[entry.id] = controlIds_.back().get(); > >>>>>>>> } > >>>>>>>> > >>>>>>>> - const ControlId *controlId = idMap->at(entry->id); > >>>>>>>> + const ControlId *controlId = idMap->at(entry.id); > >>>>>>>> ASSERT(controlId); > >>>>>>>> > >>>>>>>> - if (entry->offset != values.offset()) { > >>>>>>>> + if (entry.offset != values.offset()) { > >>>>>>>> LOG(Serializer, Error) > >>>>>>>> << "Bad data, entry offset mismatch (entry " > >>>>>>>> << i << ")"; > >>>>>>>> @@ -526,9 +525,9 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > >>>>>>>> * Create the ControlInfoMap in the cache, and store the map to handle > >>>>>>>> * association. > >>>>>>>> */ > >>>>>>>> - infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap); > >>>>>>>> - ControlInfoMap &map = infoMaps_[hdr->handle]; > >>>>>>>> - infoMapHandles_[&map] = hdr->handle; > >>>>>>>> + infoMaps_[hdr.handle] = ControlInfoMap(std::move(ctrls), *idMap); > >>>>>>>> + ControlInfoMap &map = infoMaps_[hdr.handle]; > >>>>>>>> + infoMapHandles_[&map] = hdr.handle; > >>>>>>>> > >>>>>>>> return map; > >>>>>>>> } > >>>>>>>> @@ -545,21 +544,21 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > >>>>>>>> template<> > >>>>>>>> ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer) > >>>>>>>> { > >>>>>>>> - const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); > >>>>>>>> - if (!hdr) { > >>>>>>>> + struct ipa_controls_header hdr; > >>>>>>>> + if (buffer.read(&hdr) < 0) { > >>>>>>>> LOG(Serializer, Error) << "Out of data"; > >>>>>>>> return {}; > >>>>>>>> } > >>>>>>>> > >>>>>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { > >>>>>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { > >>>>>>>> LOG(Serializer, Error) > >>>>>>>> << "Unsupported controls format version " > >>>>>>>> - << hdr->version; > >>>>>>>> + << hdr.version; > >>>>>>>> return {}; > >>>>>>>> } > >>>>>>>> > >>>>>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); > >>>>>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); > >>>>>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); > >>>>>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); > >>>>>>>> > >>>>>>>> if (buffer.overflow()) { > >>>>>>>> LOG(Serializer, Error) << "Out of data"; > >>>>>>>> @@ -576,10 +575,10 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > >>>>>>>> * ControlInfoMap is available. > >>>>>>>> */ > >>>>>>>> const ControlIdMap *idMap; > >>>>>>>> - if (hdr->handle) { > >>>>>>>> + if (hdr.handle) { > >>>>>>>> auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(), > >>>>>>>> [&](decltype(infoMapHandles_)::value_type &entry) { > >>>>>>>> - return entry.second == hdr->handle; > >>>>>>>> + return entry.second == hdr.handle; > >>>>>>>> }); > >>>>>>>> if (iter == infoMapHandles_.end()) { > >>>>>>>> LOG(Serializer, Error) > >>>>>>>> @@ -590,7 +589,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > >>>>>>>> const ControlInfoMap *infoMap = iter->first; > >>>>>>>> idMap = &infoMap->idmap(); > >>>>>>>> } else { > >>>>>>>> - switch (hdr->id_map_type) { > >>>>>>>> + switch (hdr.id_map_type) { > >>>>>>>> case IPA_CONTROL_ID_MAP_CONTROLS: > >>>>>>>> idMap = &controls::controls; > >>>>>>>> break; > >>>>>>>> @@ -601,7 +600,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > >>>>>>>> > >>>>>>>> case IPA_CONTROL_ID_MAP_V4L2: > >>>>>>>> default: > >>>>>>>> - if (hdr->entries > 0) > >>>>>>>> + if (hdr.entries > 0) > >>>>>>>> LOG(Serializer, Fatal) > >>>>>>>> << "A list of V4L2 controls requires a ControlInfoMap"; > >>>>>>>> > >>>>>>>> @@ -617,23 +616,21 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > >>>>>>>> */ > >>>>>>>> ControlList ctrls(*idMap); > >>>>>>>> > >>>>>>>> - for (unsigned int i = 0; i < hdr->entries; ++i) { > >>>>>>>> - const struct ipa_control_value_entry *entry = > >>>>>>>> - entries.read<decltype(*entry)>(); > >>>>>>>> - if (!entry) { > >>>>>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) { > >>>>>>>> + struct ipa_control_value_entry entry; > >>>>>>>> + if (entries.read(&entry) < 0) { > >>>>>>>> LOG(Serializer, Error) << "Out of data"; > >>>>>>>> return {}; > >>>>>>>> } > >>>>>>>> > >>>>>>>> - if (entry->offset != values.offset()) { > >>>>>>>> + if (entry.offset != values.offset()) { > >>>>>>>> LOG(Serializer, Error) > >>>>>>>> << "Bad data, entry offset mismatch (entry " > >>>>>>>> << i << ")"; > >>>>>>>> return {}; > >>>>>>>> } > >>>>>>>> > >>>>>>>> - ctrls.set(entry->id, > >>>>>>>> - loadControlValue(values, entry->is_array, entry->count)); > >>>>>>>> + ctrls.set(entry.id, loadControlValue(values, entry.is_array, entry.count)); > >>>>>>>> } > >>>>>>>> > >>>>>>>> return ctrls;
2025. 08. 13. 12:38 keltezéssel, Laurent Pinchart írta: > On Wed, Aug 13, 2025 at 12:32:29PM +0200, Barnabás Pőcze wrote: >> 2025. 08. 11. 17:21 keltezéssel, Laurent Pinchart írta: >>> On Mon, Aug 11, 2025 at 04:52:45PM +0200, Barnabás Pőcze wrote: >>>> 2025. 08. 11. 16:38 keltezéssel, Laurent Pinchart írta: >>>>> On Mon, Aug 11, 2025 at 04:28:10PM +0200, Barnabás Pőcze wrote: >>>>>> 2025. 08. 11. 16:23 keltezéssel, Kieran Bingham írta: >>>>>>> Quoting Barnabás Pőcze (2025-08-11 15:18:57) >>>>>>>> 2025. 08. 11. 16:12 keltezéssel, Kieran Bingham írta: >>>>>>>>> Quoting Barnabás Pőcze (2025-08-11 14:11:59) >>>>>>>>>> Aliasing an arbitrary buffer with some `T *` is unlikely to yield good >>>>>>>>>> results, primarily due to potential alignment issues. So don't do it, >>>>>>>>>> instead create the objects on the stack and read into them, this >>>>>>>>>> removes any and all strict aliasing or alignment concerns. >>>>>>>>>> >>>>>>>>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=221 >>>>>>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>>>>>>>> --- >>>>>>>>>> src/libcamera/control_serializer.cpp | 75 +++++++++++++--------------- >>>>>>>>>> 1 file changed, 36 insertions(+), 39 deletions(-) >>>>>>>>>> >>>>>>>>>> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp >>>>>>>>>> index 050f8512b..722ff231c 100644 >>>>>>>>>> --- a/src/libcamera/control_serializer.cpp >>>>>>>>>> +++ b/src/libcamera/control_serializer.cpp >>>>>>>>>> @@ -427,22 +427,22 @@ ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b) >>>>>>>>>> template<> >>>>>>>>>> ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &buffer) >>>>>>>>>> { >>>>>>>>>> - const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); >>>>>>>>>> - if (!hdr) { >>>>>>>>>> + struct ipa_controls_header hdr; >>>>>>>>>> + if (buffer.read(&hdr) < 0) { >>>>>>>>> >>>>>>>>> Aha, but the key difference is that before it was 'zero copy' just >>>>>>>>> pointing at the data, while now we're making a copy ? >>>>>>>> >>>>>>>> Yes. >>>>>>>> >>>>>>>>> I wonder if we can detect the alignment and only do the copy if >>>>>>>>> unaligned? Or maybe that's just not worth it and we should just accept >>>>>>>>> the copy - this shouldn't be a high data volume for the most parts... >>>>>>>> >>>>>>>> I feel like the control id/info map/list/value allocation easily dominates the >>>>>>>> cost of these small copies. These are all <=32 bytes. >>>>>>> >>>>>>> Hrm.. indeed - we don't normally expect ControlLists to contain large >>>>>>> data. >>>>>>> >>>>>>> I thnik CCMs with a 3x3 matrix is 'hopefully' the biggest we expect for >>>>>>> the most part and things like lens shading tables shouldn't be >>>>>>> popultated through a control list. >>>>>> >>>>>> This change only affects the headers, e.g. the bytes of the control value >>>>>> data is still read the same way, directly from the buffer. >>>>>> >>>>>>> So ... yes a copy seems quick and trivial... >>>>>>> >>>>>>> I'll put this in already: >>>>>>> >>>>>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>>>>> >>>>>>> But probably want this also checked by either or both of Laurent and >>>>>>> Paul who know the IPC mechanisms well. >>>>>>> >>>>>>>>> But maybe its something we want to be able to profile/monitor in the >>>>>>>>> future. >>>>>>>>> >>>>>>>>> Or perhaps can we find a way to align data when it goes into the >>>>>>>>> ByteStreamBuffer with some alignment padding sections somehow? >>>>>>>> >>>>>>>> I could be missing something, but given how ipc serialization is done >>>>>>>> at the moment, it does not seem entirely trivial. >>>>> >>>>> What's our alignment issue here, what field(s) are unaligned ? The >>>>> header contains 32-bit fields, are we serializing data in such a way >>>>> that causes ipa_controls_header instances to not have a 4 bytes >>>>> alignment (as in hdr.size not being a multiple of 4) ? Or is the buffer >>>>> itself not aligned ? >>>> >>>> I will turn the question around: what would guarantee the alignment? >>>> As far as I can tell, there is nothing. `ControlSerializer` does not guarantee >>>> any alignment in and of itself (e.g. `ipa_controls_header::size` can easily be >>>> not divisible by 4, e.g. `ControlSerializer::binarySize()` of a uint32_t is 5). >>>> And even if it did guarantee alignment, an appropriately sized string serialized >>>> before it can easily throw the final alignment off. >>> >>> I thought we were supposed to pad control packets to multiples of 8 >>> bytes, as described in ipa_controls.cpp. That's what surprises me here. >> >> Ahh, that's true. I haven't noticed that. How should we proceed then? I would like to >> go ahead with this change now for two reasons: >> >> * the way serialization is done does not seem to guarantee sufficient alignment even >> if the control lists, etc. were serialized respecting the alignment requirements; > > Does that mean there's a bug in the serialization code that we should > fix ? It does not match the documentation so I suppose at least one of them has to be fixed. > >> * I would like to incorporate https://patchwork.libcamera.org/patch/23373/ which >> changes at the least code appreciably, so I think it would be better to return >> to the question of alignment after that. > > I agree that this patch will not significantly hinder performance as it > only copies the headers. However, with the documented alignment, this > copy shouldn't be needed, and avoiding a copy would still be best. Are > you proposing merging this patch and https://patchwork.libcamera.org/patch/23373/, > and then fix the alignment problem and remove the copy on top ? Something like that, yes. My first impression is that getting the alignment right will need non-trivial changes, so I would postpone it until later (at least until after the other serialization related patch). Regards, Barnabás Pőcze > >>>>>>>>> If fixing/specifying the alignment in the buffer isn't possible easy - >>>>>>>>> then I could certainly imagine accepting this as a current possible solution ... >>>>>>>>> >>>>>>>>>> LOG(Serializer, Error) << "Out of data"; >>>>>>>>>> return {}; >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> - auto iter = infoMaps_.find(hdr->handle); >>>>>>>>>> + auto iter = infoMaps_.find(hdr.handle); >>>>>>>>>> if (iter != infoMaps_.end()) { >>>>>>>>>> LOG(Serializer, Debug) << "Use cached ControlInfoMap"; >>>>>>>>>> return iter->second; >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { >>>>>>>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { >>>>>>>>>> LOG(Serializer, Error) >>>>>>>>>> << "Unsupported controls format version " >>>>>>>>>> - << hdr->version; >>>>>>>>>> + << hdr.version; >>>>>>>>>> return {}; >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> @@ -455,7 +455,7 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & >>>>>>>>>> */ >>>>>>>>>> const ControlIdMap *idMap = nullptr; >>>>>>>>>> ControlIdMap *localIdMap = nullptr; >>>>>>>>>> - switch (hdr->id_map_type) { >>>>>>>>>> + switch (hdr.id_map_type) { >>>>>>>>>> case IPA_CONTROL_ID_MAP_CONTROLS: >>>>>>>>>> idMap = &controls::controls; >>>>>>>>>> break; >>>>>>>>>> @@ -469,12 +469,12 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & >>>>>>>>>> break; >>>>>>>>>> default: >>>>>>>>>> LOG(Serializer, Error) >>>>>>>>>> - << "Unknown id map type: " << hdr->id_map_type; >>>>>>>>>> + << "Unknown id map type: " << hdr.id_map_type; >>>>>>>>>> return {}; >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); >>>>>>>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); >>>>>>>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); >>>>>>>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); >>>>>>>>>> >>>>>>>>>> if (buffer.overflow()) { >>>>>>>>>> LOG(Serializer, Error) << "Out of data"; >>>>>>>>>> @@ -482,36 +482,35 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> ControlInfoMap::Map ctrls; >>>>>>>>>> - for (unsigned int i = 0; i < hdr->entries; ++i) { >>>>>>>>>> - const struct ipa_control_info_entry *entry = >>>>>>>>>> - entries.read<decltype(*entry)>(); >>>>>>>>>> - if (!entry) { >>>>>>>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) { >>>>>>>>>> + struct ipa_control_info_entry entry; >>>>>>>>>> + if (entries.read(&entry) < 0) { >>>>>>>>>> LOG(Serializer, Error) << "Out of data"; >>>>>>>>>> return {}; >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> - ControlType type = static_cast<ControlType>(entry->type); >>>>>>>>>> + ControlType type = static_cast<ControlType>(entry.type); >>>>>>>>>> >>>>>>>>>> /* If we're using a local id map, populate it. */ >>>>>>>>>> if (localIdMap) { >>>>>>>>>> ControlId::DirectionFlags flags{ >>>>>>>>>> - static_cast<ControlId::Direction>(entry->direction) >>>>>>>>>> + static_cast<ControlId::Direction>(entry.direction) >>>>>>>>>> }; >>>>>>>>>> >>>>>>>>>> /** >>>>>>>>>> * \todo Find a way to preserve the control name for >>>>>>>>>> * debugging purpose. >>>>>>>>>> */ >>>>>>>>>> - controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, >>>>>>>>>> + controlIds_.emplace_back(std::make_unique<ControlId>(entry.id, >>>>>>>>>> "", "local", type, >>>>>>>>>> flags)); >>>>>>>>>> - (*localIdMap)[entry->id] = controlIds_.back().get(); >>>>>>>>>> + (*localIdMap)[entry.id] = controlIds_.back().get(); >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> - const ControlId *controlId = idMap->at(entry->id); >>>>>>>>>> + const ControlId *controlId = idMap->at(entry.id); >>>>>>>>>> ASSERT(controlId); >>>>>>>>>> >>>>>>>>>> - if (entry->offset != values.offset()) { >>>>>>>>>> + if (entry.offset != values.offset()) { >>>>>>>>>> LOG(Serializer, Error) >>>>>>>>>> << "Bad data, entry offset mismatch (entry " >>>>>>>>>> << i << ")"; >>>>>>>>>> @@ -526,9 +525,9 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & >>>>>>>>>> * Create the ControlInfoMap in the cache, and store the map to handle >>>>>>>>>> * association. >>>>>>>>>> */ >>>>>>>>>> - infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap); >>>>>>>>>> - ControlInfoMap &map = infoMaps_[hdr->handle]; >>>>>>>>>> - infoMapHandles_[&map] = hdr->handle; >>>>>>>>>> + infoMaps_[hdr.handle] = ControlInfoMap(std::move(ctrls), *idMap); >>>>>>>>>> + ControlInfoMap &map = infoMaps_[hdr.handle]; >>>>>>>>>> + infoMapHandles_[&map] = hdr.handle; >>>>>>>>>> >>>>>>>>>> return map; >>>>>>>>>> } >>>>>>>>>> @@ -545,21 +544,21 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & >>>>>>>>>> template<> >>>>>>>>>> ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer) >>>>>>>>>> { >>>>>>>>>> - const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); >>>>>>>>>> - if (!hdr) { >>>>>>>>>> + struct ipa_controls_header hdr; >>>>>>>>>> + if (buffer.read(&hdr) < 0) { >>>>>>>>>> LOG(Serializer, Error) << "Out of data"; >>>>>>>>>> return {}; >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { >>>>>>>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { >>>>>>>>>> LOG(Serializer, Error) >>>>>>>>>> << "Unsupported controls format version " >>>>>>>>>> - << hdr->version; >>>>>>>>>> + << hdr.version; >>>>>>>>>> return {}; >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); >>>>>>>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); >>>>>>>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); >>>>>>>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); >>>>>>>>>> >>>>>>>>>> if (buffer.overflow()) { >>>>>>>>>> LOG(Serializer, Error) << "Out of data"; >>>>>>>>>> @@ -576,10 +575,10 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer >>>>>>>>>> * ControlInfoMap is available. >>>>>>>>>> */ >>>>>>>>>> const ControlIdMap *idMap; >>>>>>>>>> - if (hdr->handle) { >>>>>>>>>> + if (hdr.handle) { >>>>>>>>>> auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(), >>>>>>>>>> [&](decltype(infoMapHandles_)::value_type &entry) { >>>>>>>>>> - return entry.second == hdr->handle; >>>>>>>>>> + return entry.second == hdr.handle; >>>>>>>>>> }); >>>>>>>>>> if (iter == infoMapHandles_.end()) { >>>>>>>>>> LOG(Serializer, Error) >>>>>>>>>> @@ -590,7 +589,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer >>>>>>>>>> const ControlInfoMap *infoMap = iter->first; >>>>>>>>>> idMap = &infoMap->idmap(); >>>>>>>>>> } else { >>>>>>>>>> - switch (hdr->id_map_type) { >>>>>>>>>> + switch (hdr.id_map_type) { >>>>>>>>>> case IPA_CONTROL_ID_MAP_CONTROLS: >>>>>>>>>> idMap = &controls::controls; >>>>>>>>>> break; >>>>>>>>>> @@ -601,7 +600,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer >>>>>>>>>> >>>>>>>>>> case IPA_CONTROL_ID_MAP_V4L2: >>>>>>>>>> default: >>>>>>>>>> - if (hdr->entries > 0) >>>>>>>>>> + if (hdr.entries > 0) >>>>>>>>>> LOG(Serializer, Fatal) >>>>>>>>>> << "A list of V4L2 controls requires a ControlInfoMap"; >>>>>>>>>> >>>>>>>>>> @@ -617,23 +616,21 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer >>>>>>>>>> */ >>>>>>>>>> ControlList ctrls(*idMap); >>>>>>>>>> >>>>>>>>>> - for (unsigned int i = 0; i < hdr->entries; ++i) { >>>>>>>>>> - const struct ipa_control_value_entry *entry = >>>>>>>>>> - entries.read<decltype(*entry)>(); >>>>>>>>>> - if (!entry) { >>>>>>>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) { >>>>>>>>>> + struct ipa_control_value_entry entry; >>>>>>>>>> + if (entries.read(&entry) < 0) { >>>>>>>>>> LOG(Serializer, Error) << "Out of data"; >>>>>>>>>> return {}; >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> - if (entry->offset != values.offset()) { >>>>>>>>>> + if (entry.offset != values.offset()) { >>>>>>>>>> LOG(Serializer, Error) >>>>>>>>>> << "Bad data, entry offset mismatch (entry " >>>>>>>>>> << i << ")"; >>>>>>>>>> return {}; >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> - ctrls.set(entry->id, >>>>>>>>>> - loadControlValue(values, entry->is_array, entry->count)); >>>>>>>>>> + ctrls.set(entry.id, loadControlValue(values, entry.is_array, entry.count)); >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> return ctrls; >
On Wed, Aug 13, 2025 at 12:48:20PM +0200, Barnabás Pőcze wrote: > 2025. 08. 13. 12:38 keltezéssel, Laurent Pinchart írta: > > On Wed, Aug 13, 2025 at 12:32:29PM +0200, Barnabás Pőcze wrote: > >> 2025. 08. 11. 17:21 keltezéssel, Laurent Pinchart írta: > >>> On Mon, Aug 11, 2025 at 04:52:45PM +0200, Barnabás Pőcze wrote: > >>>> 2025. 08. 11. 16:38 keltezéssel, Laurent Pinchart írta: > >>>>> On Mon, Aug 11, 2025 at 04:28:10PM +0200, Barnabás Pőcze wrote: > >>>>>> 2025. 08. 11. 16:23 keltezéssel, Kieran Bingham írta: > >>>>>>> Quoting Barnabás Pőcze (2025-08-11 15:18:57) > >>>>>>>> 2025. 08. 11. 16:12 keltezéssel, Kieran Bingham írta: > >>>>>>>>> Quoting Barnabás Pőcze (2025-08-11 14:11:59) > >>>>>>>>>> Aliasing an arbitrary buffer with some `T *` is unlikely to yield good > >>>>>>>>>> results, primarily due to potential alignment issues. So don't do it, > >>>>>>>>>> instead create the objects on the stack and read into them, this > >>>>>>>>>> removes any and all strict aliasing or alignment concerns. > >>>>>>>>>> > >>>>>>>>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=221 > >>>>>>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >>>>>>>>>> --- > >>>>>>>>>> src/libcamera/control_serializer.cpp | 75 +++++++++++++--------------- > >>>>>>>>>> 1 file changed, 36 insertions(+), 39 deletions(-) > >>>>>>>>>> > >>>>>>>>>> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > >>>>>>>>>> index 050f8512b..722ff231c 100644 > >>>>>>>>>> --- a/src/libcamera/control_serializer.cpp > >>>>>>>>>> +++ b/src/libcamera/control_serializer.cpp > >>>>>>>>>> @@ -427,22 +427,22 @@ ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b) > >>>>>>>>>> template<> > >>>>>>>>>> ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &buffer) > >>>>>>>>>> { > >>>>>>>>>> - const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); > >>>>>>>>>> - if (!hdr) { > >>>>>>>>>> + struct ipa_controls_header hdr; > >>>>>>>>>> + if (buffer.read(&hdr) < 0) { > >>>>>>>>> > >>>>>>>>> Aha, but the key difference is that before it was 'zero copy' just > >>>>>>>>> pointing at the data, while now we're making a copy ? > >>>>>>>> > >>>>>>>> Yes. > >>>>>>>> > >>>>>>>>> I wonder if we can detect the alignment and only do the copy if > >>>>>>>>> unaligned? Or maybe that's just not worth it and we should just accept > >>>>>>>>> the copy - this shouldn't be a high data volume for the most parts... > >>>>>>>> > >>>>>>>> I feel like the control id/info map/list/value allocation easily dominates the > >>>>>>>> cost of these small copies. These are all <=32 bytes. > >>>>>>> > >>>>>>> Hrm.. indeed - we don't normally expect ControlLists to contain large > >>>>>>> data. > >>>>>>> > >>>>>>> I thnik CCMs with a 3x3 matrix is 'hopefully' the biggest we expect for > >>>>>>> the most part and things like lens shading tables shouldn't be > >>>>>>> popultated through a control list. > >>>>>> > >>>>>> This change only affects the headers, e.g. the bytes of the control value > >>>>>> data is still read the same way, directly from the buffer. > >>>>>> > >>>>>>> So ... yes a copy seems quick and trivial... > >>>>>>> > >>>>>>> I'll put this in already: > >>>>>>> > >>>>>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>>>>>> > >>>>>>> But probably want this also checked by either or both of Laurent and > >>>>>>> Paul who know the IPC mechanisms well. > >>>>>>> > >>>>>>>>> But maybe its something we want to be able to profile/monitor in the > >>>>>>>>> future. > >>>>>>>>> > >>>>>>>>> Or perhaps can we find a way to align data when it goes into the > >>>>>>>>> ByteStreamBuffer with some alignment padding sections somehow? > >>>>>>>> > >>>>>>>> I could be missing something, but given how ipc serialization is done > >>>>>>>> at the moment, it does not seem entirely trivial. > >>>>> > >>>>> What's our alignment issue here, what field(s) are unaligned ? The > >>>>> header contains 32-bit fields, are we serializing data in such a way > >>>>> that causes ipa_controls_header instances to not have a 4 bytes > >>>>> alignment (as in hdr.size not being a multiple of 4) ? Or is the buffer > >>>>> itself not aligned ? > >>>> > >>>> I will turn the question around: what would guarantee the alignment? > >>>> As far as I can tell, there is nothing. `ControlSerializer` does not guarantee > >>>> any alignment in and of itself (e.g. `ipa_controls_header::size` can easily be > >>>> not divisible by 4, e.g. `ControlSerializer::binarySize()` of a uint32_t is 5). > >>>> And even if it did guarantee alignment, an appropriately sized string serialized > >>>> before it can easily throw the final alignment off. > >>> > >>> I thought we were supposed to pad control packets to multiples of 8 > >>> bytes, as described in ipa_controls.cpp. That's what surprises me here. > >> > >> Ahh, that's true. I haven't noticed that. How should we proceed then? I would like to > >> go ahead with this change now for two reasons: > >> > >> * the way serialization is done does not seem to guarantee sufficient alignment even > >> if the control lists, etc. were serialized respecting the alignment requirements; > > > > Does that mean there's a bug in the serialization code that we should > > fix ? > > It does not match the documentation so I suppose at least one of them has to be fixed. Indeed :-) > >> * I would like to incorporate https://patchwork.libcamera.org/patch/23373/ which > >> changes at the least code appreciably, so I think it would be better to return > >> to the question of alignment after that. > > > > I agree that this patch will not significantly hinder performance as it > > only copies the headers. However, with the documented alignment, this > > copy shouldn't be needed, and avoiding a copy would still be best. Are > > you proposing merging this patch and https://patchwork.libcamera.org/patch/23373/, > > and then fix the alignment problem and remove the copy on top ? > > Something like that, yes. My first impression is that getting the alignment right will > need non-trivial changes, so I would postpone it until later (at least until after the > other serialization related patch). I haven't looked at what it would take to fix the alignment, so I don't have a clear opinion. If you think it's easier to address it on top, I'm OK with that, even if it will cause a bit of churn going back and forth with the copy. > >>>>>>>>> If fixing/specifying the alignment in the buffer isn't possible easy - > >>>>>>>>> then I could certainly imagine accepting this as a current possible solution ... > >>>>>>>>> > >>>>>>>>>> LOG(Serializer, Error) << "Out of data"; > >>>>>>>>>> return {}; > >>>>>>>>>> } > >>>>>>>>>> > >>>>>>>>>> - auto iter = infoMaps_.find(hdr->handle); > >>>>>>>>>> + auto iter = infoMaps_.find(hdr.handle); > >>>>>>>>>> if (iter != infoMaps_.end()) { > >>>>>>>>>> LOG(Serializer, Debug) << "Use cached ControlInfoMap"; > >>>>>>>>>> return iter->second; > >>>>>>>>>> } > >>>>>>>>>> > >>>>>>>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { > >>>>>>>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { > >>>>>>>>>> LOG(Serializer, Error) > >>>>>>>>>> << "Unsupported controls format version " > >>>>>>>>>> - << hdr->version; > >>>>>>>>>> + << hdr.version; > >>>>>>>>>> return {}; > >>>>>>>>>> } > >>>>>>>>>> > >>>>>>>>>> @@ -455,7 +455,7 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > >>>>>>>>>> */ > >>>>>>>>>> const ControlIdMap *idMap = nullptr; > >>>>>>>>>> ControlIdMap *localIdMap = nullptr; > >>>>>>>>>> - switch (hdr->id_map_type) { > >>>>>>>>>> + switch (hdr.id_map_type) { > >>>>>>>>>> case IPA_CONTROL_ID_MAP_CONTROLS: > >>>>>>>>>> idMap = &controls::controls; > >>>>>>>>>> break; > >>>>>>>>>> @@ -469,12 +469,12 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > >>>>>>>>>> break; > >>>>>>>>>> default: > >>>>>>>>>> LOG(Serializer, Error) > >>>>>>>>>> - << "Unknown id map type: " << hdr->id_map_type; > >>>>>>>>>> + << "Unknown id map type: " << hdr.id_map_type; > >>>>>>>>>> return {}; > >>>>>>>>>> } > >>>>>>>>>> > >>>>>>>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); > >>>>>>>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); > >>>>>>>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); > >>>>>>>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); > >>>>>>>>>> > >>>>>>>>>> if (buffer.overflow()) { > >>>>>>>>>> LOG(Serializer, Error) << "Out of data"; > >>>>>>>>>> @@ -482,36 +482,35 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > >>>>>>>>>> } > >>>>>>>>>> > >>>>>>>>>> ControlInfoMap::Map ctrls; > >>>>>>>>>> - for (unsigned int i = 0; i < hdr->entries; ++i) { > >>>>>>>>>> - const struct ipa_control_info_entry *entry = > >>>>>>>>>> - entries.read<decltype(*entry)>(); > >>>>>>>>>> - if (!entry) { > >>>>>>>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) { > >>>>>>>>>> + struct ipa_control_info_entry entry; > >>>>>>>>>> + if (entries.read(&entry) < 0) { > >>>>>>>>>> LOG(Serializer, Error) << "Out of data"; > >>>>>>>>>> return {}; > >>>>>>>>>> } > >>>>>>>>>> > >>>>>>>>>> - ControlType type = static_cast<ControlType>(entry->type); > >>>>>>>>>> + ControlType type = static_cast<ControlType>(entry.type); > >>>>>>>>>> > >>>>>>>>>> /* If we're using a local id map, populate it. */ > >>>>>>>>>> if (localIdMap) { > >>>>>>>>>> ControlId::DirectionFlags flags{ > >>>>>>>>>> - static_cast<ControlId::Direction>(entry->direction) > >>>>>>>>>> + static_cast<ControlId::Direction>(entry.direction) > >>>>>>>>>> }; > >>>>>>>>>> > >>>>>>>>>> /** > >>>>>>>>>> * \todo Find a way to preserve the control name for > >>>>>>>>>> * debugging purpose. > >>>>>>>>>> */ > >>>>>>>>>> - controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, > >>>>>>>>>> + controlIds_.emplace_back(std::make_unique<ControlId>(entry.id, > >>>>>>>>>> "", "local", type, > >>>>>>>>>> flags)); > >>>>>>>>>> - (*localIdMap)[entry->id] = controlIds_.back().get(); > >>>>>>>>>> + (*localIdMap)[entry.id] = controlIds_.back().get(); > >>>>>>>>>> } > >>>>>>>>>> > >>>>>>>>>> - const ControlId *controlId = idMap->at(entry->id); > >>>>>>>>>> + const ControlId *controlId = idMap->at(entry.id); > >>>>>>>>>> ASSERT(controlId); > >>>>>>>>>> > >>>>>>>>>> - if (entry->offset != values.offset()) { > >>>>>>>>>> + if (entry.offset != values.offset()) { > >>>>>>>>>> LOG(Serializer, Error) > >>>>>>>>>> << "Bad data, entry offset mismatch (entry " > >>>>>>>>>> << i << ")"; > >>>>>>>>>> @@ -526,9 +525,9 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > >>>>>>>>>> * Create the ControlInfoMap in the cache, and store the map to handle > >>>>>>>>>> * association. > >>>>>>>>>> */ > >>>>>>>>>> - infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap); > >>>>>>>>>> - ControlInfoMap &map = infoMaps_[hdr->handle]; > >>>>>>>>>> - infoMapHandles_[&map] = hdr->handle; > >>>>>>>>>> + infoMaps_[hdr.handle] = ControlInfoMap(std::move(ctrls), *idMap); > >>>>>>>>>> + ControlInfoMap &map = infoMaps_[hdr.handle]; > >>>>>>>>>> + infoMapHandles_[&map] = hdr.handle; > >>>>>>>>>> > >>>>>>>>>> return map; > >>>>>>>>>> } > >>>>>>>>>> @@ -545,21 +544,21 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > >>>>>>>>>> template<> > >>>>>>>>>> ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer) > >>>>>>>>>> { > >>>>>>>>>> - const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); > >>>>>>>>>> - if (!hdr) { > >>>>>>>>>> + struct ipa_controls_header hdr; > >>>>>>>>>> + if (buffer.read(&hdr) < 0) { > >>>>>>>>>> LOG(Serializer, Error) << "Out of data"; > >>>>>>>>>> return {}; > >>>>>>>>>> } > >>>>>>>>>> > >>>>>>>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { > >>>>>>>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { > >>>>>>>>>> LOG(Serializer, Error) > >>>>>>>>>> << "Unsupported controls format version " > >>>>>>>>>> - << hdr->version; > >>>>>>>>>> + << hdr.version; > >>>>>>>>>> return {}; > >>>>>>>>>> } > >>>>>>>>>> > >>>>>>>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); > >>>>>>>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); > >>>>>>>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); > >>>>>>>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); > >>>>>>>>>> > >>>>>>>>>> if (buffer.overflow()) { > >>>>>>>>>> LOG(Serializer, Error) << "Out of data"; > >>>>>>>>>> @@ -576,10 +575,10 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > >>>>>>>>>> * ControlInfoMap is available. > >>>>>>>>>> */ > >>>>>>>>>> const ControlIdMap *idMap; > >>>>>>>>>> - if (hdr->handle) { > >>>>>>>>>> + if (hdr.handle) { > >>>>>>>>>> auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(), > >>>>>>>>>> [&](decltype(infoMapHandles_)::value_type &entry) { > >>>>>>>>>> - return entry.second == hdr->handle; > >>>>>>>>>> + return entry.second == hdr.handle; > >>>>>>>>>> }); > >>>>>>>>>> if (iter == infoMapHandles_.end()) { > >>>>>>>>>> LOG(Serializer, Error) > >>>>>>>>>> @@ -590,7 +589,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > >>>>>>>>>> const ControlInfoMap *infoMap = iter->first; > >>>>>>>>>> idMap = &infoMap->idmap(); > >>>>>>>>>> } else { > >>>>>>>>>> - switch (hdr->id_map_type) { > >>>>>>>>>> + switch (hdr.id_map_type) { > >>>>>>>>>> case IPA_CONTROL_ID_MAP_CONTROLS: > >>>>>>>>>> idMap = &controls::controls; > >>>>>>>>>> break; > >>>>>>>>>> @@ -601,7 +600,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > >>>>>>>>>> > >>>>>>>>>> case IPA_CONTROL_ID_MAP_V4L2: > >>>>>>>>>> default: > >>>>>>>>>> - if (hdr->entries > 0) > >>>>>>>>>> + if (hdr.entries > 0) > >>>>>>>>>> LOG(Serializer, Fatal) > >>>>>>>>>> << "A list of V4L2 controls requires a ControlInfoMap"; > >>>>>>>>>> > >>>>>>>>>> @@ -617,23 +616,21 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer > >>>>>>>>>> */ > >>>>>>>>>> ControlList ctrls(*idMap); > >>>>>>>>>> > >>>>>>>>>> - for (unsigned int i = 0; i < hdr->entries; ++i) { > >>>>>>>>>> - const struct ipa_control_value_entry *entry = > >>>>>>>>>> - entries.read<decltype(*entry)>(); > >>>>>>>>>> - if (!entry) { > >>>>>>>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) { > >>>>>>>>>> + struct ipa_control_value_entry entry; > >>>>>>>>>> + if (entries.read(&entry) < 0) { > >>>>>>>>>> LOG(Serializer, Error) << "Out of data"; > >>>>>>>>>> return {}; > >>>>>>>>>> } > >>>>>>>>>> > >>>>>>>>>> - if (entry->offset != values.offset()) { > >>>>>>>>>> + if (entry.offset != values.offset()) { > >>>>>>>>>> LOG(Serializer, Error) > >>>>>>>>>> << "Bad data, entry offset mismatch (entry " > >>>>>>>>>> << i << ")"; > >>>>>>>>>> return {}; > >>>>>>>>>> } > >>>>>>>>>> > >>>>>>>>>> - ctrls.set(entry->id, > >>>>>>>>>> - loadControlValue(values, entry->is_array, entry->count)); > >>>>>>>>>> + ctrls.set(entry.id, loadControlValue(values, entry.is_array, entry.count)); > >>>>>>>>>> } > >>>>>>>>>> > >>>>>>>>>> return ctrls;
diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp index 050f8512b..722ff231c 100644 --- a/src/libcamera/control_serializer.cpp +++ b/src/libcamera/control_serializer.cpp @@ -427,22 +427,22 @@ ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b) template<> ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &buffer) { - const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); - if (!hdr) { + struct ipa_controls_header hdr; + if (buffer.read(&hdr) < 0) { LOG(Serializer, Error) << "Out of data"; return {}; } - auto iter = infoMaps_.find(hdr->handle); + auto iter = infoMaps_.find(hdr.handle); if (iter != infoMaps_.end()) { LOG(Serializer, Debug) << "Use cached ControlInfoMap"; return iter->second; } - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { LOG(Serializer, Error) << "Unsupported controls format version " - << hdr->version; + << hdr.version; return {}; } @@ -455,7 +455,7 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & */ const ControlIdMap *idMap = nullptr; ControlIdMap *localIdMap = nullptr; - switch (hdr->id_map_type) { + switch (hdr.id_map_type) { case IPA_CONTROL_ID_MAP_CONTROLS: idMap = &controls::controls; break; @@ -469,12 +469,12 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & break; default: LOG(Serializer, Error) - << "Unknown id map type: " << hdr->id_map_type; + << "Unknown id map type: " << hdr.id_map_type; return {}; } - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); if (buffer.overflow()) { LOG(Serializer, Error) << "Out of data"; @@ -482,36 +482,35 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & } ControlInfoMap::Map ctrls; - for (unsigned int i = 0; i < hdr->entries; ++i) { - const struct ipa_control_info_entry *entry = - entries.read<decltype(*entry)>(); - if (!entry) { + for (unsigned int i = 0; i < hdr.entries; ++i) { + struct ipa_control_info_entry entry; + if (entries.read(&entry) < 0) { LOG(Serializer, Error) << "Out of data"; return {}; } - ControlType type = static_cast<ControlType>(entry->type); + ControlType type = static_cast<ControlType>(entry.type); /* If we're using a local id map, populate it. */ if (localIdMap) { ControlId::DirectionFlags flags{ - static_cast<ControlId::Direction>(entry->direction) + static_cast<ControlId::Direction>(entry.direction) }; /** * \todo Find a way to preserve the control name for * debugging purpose. */ - controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, + controlIds_.emplace_back(std::make_unique<ControlId>(entry.id, "", "local", type, flags)); - (*localIdMap)[entry->id] = controlIds_.back().get(); + (*localIdMap)[entry.id] = controlIds_.back().get(); } - const ControlId *controlId = idMap->at(entry->id); + const ControlId *controlId = idMap->at(entry.id); ASSERT(controlId); - if (entry->offset != values.offset()) { + if (entry.offset != values.offset()) { LOG(Serializer, Error) << "Bad data, entry offset mismatch (entry " << i << ")"; @@ -526,9 +525,9 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & * Create the ControlInfoMap in the cache, and store the map to handle * association. */ - infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap); - ControlInfoMap &map = infoMaps_[hdr->handle]; - infoMapHandles_[&map] = hdr->handle; + infoMaps_[hdr.handle] = ControlInfoMap(std::move(ctrls), *idMap); + ControlInfoMap &map = infoMaps_[hdr.handle]; + infoMapHandles_[&map] = hdr.handle; return map; } @@ -545,21 +544,21 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & template<> ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer) { - const struct ipa_controls_header *hdr = buffer.read<decltype(*hdr)>(); - if (!hdr) { + struct ipa_controls_header hdr; + if (buffer.read(&hdr) < 0) { LOG(Serializer, Error) << "Out of data"; return {}; } - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) { + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) { LOG(Serializer, Error) << "Unsupported controls format version " - << hdr->version; + << hdr.version; return {}; } - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr)); - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset); + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr)); + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset); if (buffer.overflow()) { LOG(Serializer, Error) << "Out of data"; @@ -576,10 +575,10 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer * ControlInfoMap is available. */ const ControlIdMap *idMap; - if (hdr->handle) { + if (hdr.handle) { auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(), [&](decltype(infoMapHandles_)::value_type &entry) { - return entry.second == hdr->handle; + return entry.second == hdr.handle; }); if (iter == infoMapHandles_.end()) { LOG(Serializer, Error) @@ -590,7 +589,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer const ControlInfoMap *infoMap = iter->first; idMap = &infoMap->idmap(); } else { - switch (hdr->id_map_type) { + switch (hdr.id_map_type) { case IPA_CONTROL_ID_MAP_CONTROLS: idMap = &controls::controls; break; @@ -601,7 +600,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer case IPA_CONTROL_ID_MAP_V4L2: default: - if (hdr->entries > 0) + if (hdr.entries > 0) LOG(Serializer, Fatal) << "A list of V4L2 controls requires a ControlInfoMap"; @@ -617,23 +616,21 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer */ ControlList ctrls(*idMap); - for (unsigned int i = 0; i < hdr->entries; ++i) { - const struct ipa_control_value_entry *entry = - entries.read<decltype(*entry)>(); - if (!entry) { + for (unsigned int i = 0; i < hdr.entries; ++i) { + struct ipa_control_value_entry entry; + if (entries.read(&entry) < 0) { LOG(Serializer, Error) << "Out of data"; return {}; } - if (entry->offset != values.offset()) { + if (entry.offset != values.offset()) { LOG(Serializer, Error) << "Bad data, entry offset mismatch (entry " << i << ")"; return {}; } - ctrls.set(entry->id, - loadControlValue(values, entry->is_array, entry->count)); + ctrls.set(entry.id, loadControlValue(values, entry.is_array, entry.count)); } return ctrls;
Aliasing an arbitrary buffer with some `T *` is unlikely to yield good results, primarily due to potential alignment issues. So don't do it, instead create the objects on the stack and read into them, this removes any and all strict aliasing or alignment concerns. Bug: https://bugs.libcamera.org/show_bug.cgi?id=221 Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/libcamera/control_serializer.cpp | 75 +++++++++++++--------------- 1 file changed, 36 insertions(+), 39 deletions(-)