[PATCH] libcamera: mtkisp7: Change ipa_control_value_entry.count to 32 bits

[Cheng-Hao Yang]

Hi Laurent,

On Fri, Oct 18, 2024 at 8:50 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Harvey, Xing-Gu,
>
> Thank you for the patch.
>
> On Fri, Oct 18, 2024 at 07:52:37AM +0000, Harvey Yang wrote:
> > From: Xing-Gu Chen <xinggu at chromium.org>
> >
> > Change ipa_control_value_entry.count to uint32_t because the
> > element count of JpegApplicationSegmentContent is bigger than
> > 65536.
>
> Transporting large data over controls is inefficients, as control lists
> get copied multiple time with isolated IPA modules. Is this a sign we
> need to think about a more efficient mechanism ? I'm also thinking we
> should review how JpegApplicationSegmentContent is handled first.

You mean serializing (multiple resizes as well) and deserializing?
Do we consider using DMA buffer to pass this for instance?
Or libcamera::SharedMem.

I also just realized that JpegApplicationSegmentContent was added
by us [1]. I found that this is only needed for a debugging module in
mtkisp7, which we might not end up upstreaming.
Do you think JpegApplicationSegmentContent makes sense and it's
something that upstream libcamera would like to have?
If this is controversial, we can also skip this for now.

BR,
Harvey

[1]: https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5233122


>
> > Signed-off-by: Xing-Gu Chen <xinggu at chromium.org>
> > Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > ---
> >  include/libcamera/ipa/ipa_controls.h | 2 +-
> >  src/libcamera/ipa_controls.cpp       | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h
> > index 5fd13394f..7a8051695 100644
> > --- a/include/libcamera/ipa/ipa_controls.h
> > +++ b/include/libcamera/ipa/ipa_controls.h
> > @@ -37,7 +37,7 @@ struct ipa_control_value_entry {
> >       uint32_t id;
> >       uint8_t type;
> >       uint8_t is_array;
> > -     uint16_t count;
> > +     uint32_t count;
> >       uint32_t offset;
> >       uint32_t padding[1];
> >  };
> > diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp
> > index 9420c889f..a1ccc7d61 100644
> > --- a/src/libcamera/ipa_controls.cpp
> > +++ b/src/libcamera/ipa_controls.cpp
> > @@ -207,7 +207,7 @@ static_assert(sizeof(ipa_controls_header) == 32,
> >   * Padding bytes (shall be set to 0)
> >   */
> >
> > -static_assert(sizeof(ipa_control_value_entry) == 16,
> > +static_assert(sizeof(ipa_control_value_entry) == 20,
> >             "Invalid ABI size change for struct ipa_control_value_entry");
> >
> >  /**
>
> --
> Regards,
>
> Laurent Pinchart