| Message ID | 20200229164254.23604-22-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On 29/02/2020 16:42, Laurent Pinchart wrote: > Report in a new field of the ipa_control_value_entry structure if the > value contains an array. Reorganize the other fields of the structure to > avoid increasing its size. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> With comments below addressed: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > include/ipa/ipa_controls.h | 6 ++++-- > src/libcamera/ipa_controls.cpp | 4 ++++ > 2 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/include/ipa/ipa_controls.h b/include/ipa/ipa_controls.h > index 6371e34575f2..37f97d6ad2a4 100644 > --- a/include/ipa/ipa_controls.h > +++ b/include/ipa/ipa_controls.h > @@ -26,9 +26,11 @@ struct ipa_controls_header { > > struct ipa_control_value_entry { > uint32_t id; > - uint32_t type; > - uint32_t count; > + uint8_t type; > + uint8_t is_array; > + uint16_t count; > uint32_t offset; > + uint32_t padding[1]; Why the [1]? Do you plan to increase to provide extra padding? If so why not do it now? Otherwise uint32_t padding would be sufficient right ? > }; > > struct ipa_control_range_entry { > diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp > index 25f38bab3dd9..cfb5c59dd028 100644 > --- a/src/libcamera/ipa_controls.cpp > +++ b/src/libcamera/ipa_controls.cpp > @@ -163,11 +163,15 @@ static_assert(sizeof(ipa_controls_header) == 32, > * The numerical ID of the control > * \var ipa_control_value_entry::type > * The type of the control (defined by enum ControlType) > + * \var ipa_control_value_entry::is_array > + * True if the control value stores an array, false otherwise > * \var ipa_control_value_entry::count Shouldn't count move to match the ordering in the struct? > * The number of control array entries for array controls (1 otherwise) > * \var ipa_control_value_entry::offset > * The offset in bytes from the beginning of the data section to the control > * value data (shall be a multiple of 8 bytes). > + * \var ipa_control_value_entry::padding > + * Padding bytes (shall be set to 0) > */ > > static_assert(sizeof(ipa_control_value_entry) == 16, >
Hi Kieran, On Thu, Mar 05, 2020 at 03:28:45PM +0000, Kieran Bingham wrote: > On 29/02/2020 16:42, Laurent Pinchart wrote: > > Report in a new field of the ipa_control_value_entry structure if the > > value contains an array. Reorganize the other fields of the structure to > > avoid increasing its size. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > With comments below addressed: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > include/ipa/ipa_controls.h | 6 ++++-- > > src/libcamera/ipa_controls.cpp | 4 ++++ > > 2 files changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/include/ipa/ipa_controls.h b/include/ipa/ipa_controls.h > > index 6371e34575f2..37f97d6ad2a4 100644 > > --- a/include/ipa/ipa_controls.h > > +++ b/include/ipa/ipa_controls.h > > @@ -26,9 +26,11 @@ struct ipa_controls_header { > > > > struct ipa_control_value_entry { > > uint32_t id; > > - uint32_t type; > > - uint32_t count; > > + uint8_t type; > > + uint8_t is_array; > > + uint16_t count; > > uint32_t offset; > > + uint32_t padding[1]; > > Why the [1]? Do you plan to increase to provide extra padding? > If so why not do it now? > > Otherwise uint32_t padding would be sufficient right ? That's because padding ipa_control_range_entry is already declared as padding[1]. I think we'll rework these structures later anyway, at which point we'll define how much padding we need/want. > > }; > > > > struct ipa_control_range_entry { > > diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp > > index 25f38bab3dd9..cfb5c59dd028 100644 > > --- a/src/libcamera/ipa_controls.cpp > > +++ b/src/libcamera/ipa_controls.cpp > > @@ -163,11 +163,15 @@ static_assert(sizeof(ipa_controls_header) == 32, > > * The numerical ID of the control > > * \var ipa_control_value_entry::type > > * The type of the control (defined by enum ControlType) > > + * \var ipa_control_value_entry::is_array > > + * True if the control value stores an array, false otherwise > > * \var ipa_control_value_entry::count > > Shouldn't count move to match the ordering in the struct? The order hasn't changed :-) > > * The number of control array entries for array controls (1 otherwise) > > * \var ipa_control_value_entry::offset > > * The offset in bytes from the beginning of the data section to the control > > * value data (shall be a multiple of 8 bytes). > > + * \var ipa_control_value_entry::padding > > + * Padding bytes (shall be set to 0) > > */ > > > > static_assert(sizeof(ipa_control_value_entry) == 16,
diff --git a/include/ipa/ipa_controls.h b/include/ipa/ipa_controls.h index 6371e34575f2..37f97d6ad2a4 100644 --- a/include/ipa/ipa_controls.h +++ b/include/ipa/ipa_controls.h @@ -26,9 +26,11 @@ struct ipa_controls_header { struct ipa_control_value_entry { uint32_t id; - uint32_t type; - uint32_t count; + uint8_t type; + uint8_t is_array; + uint16_t count; uint32_t offset; + uint32_t padding[1]; }; struct ipa_control_range_entry { diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp index 25f38bab3dd9..cfb5c59dd028 100644 --- a/src/libcamera/ipa_controls.cpp +++ b/src/libcamera/ipa_controls.cpp @@ -163,11 +163,15 @@ static_assert(sizeof(ipa_controls_header) == 32, * The numerical ID of the control * \var ipa_control_value_entry::type * The type of the control (defined by enum ControlType) + * \var ipa_control_value_entry::is_array + * True if the control value stores an array, false otherwise * \var ipa_control_value_entry::count * The number of control array entries for array controls (1 otherwise) * \var ipa_control_value_entry::offset * The offset in bytes from the beginning of the data section to the control * value data (shall be a multiple of 8 bytes). + * \var ipa_control_value_entry::padding + * Padding bytes (shall be set to 0) */ static_assert(sizeof(ipa_control_value_entry) == 16,
Report in a new field of the ipa_control_value_entry structure if the value contains an array. Reorganize the other fields of the structure to avoid increasing its size. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/ipa/ipa_controls.h | 6 ++++-- src/libcamera/ipa_controls.cpp | 4 ++++ 2 files changed, 8 insertions(+), 2 deletions(-)