[libcamera-devel] [PATCH 21/31] libcamera: ipa: Support array controls in ipa_control_value_entry

[Laurent Pinchart]

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 at ideasonboard.com>
> 
> With comments below addressed:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at 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,

-- 
Regards,

Laurent Pinchart