libcamera: mtkisp7: Change ipa_control_value_entry.count to 32 bits

21664 diff mbox series

From: Xing-Gu Chen <xinggu@chromium.org>

Change ipa_control_value_entry.count to uint32_t because the
element count of JpegApplicationSegmentContent is bigger than
65536.

Signed-off-by: Xing-Gu Chen <xinggu@chromium.org>
Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
---
 include/libcamera/ipa/ipa_controls.h | 2 +-
 src/libcamera/ipa_controls.cpp       | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Sorry, should've removed the prefix "mtkisp7".
Will fix it in the next version.

On Fri, Oct 18, 2024 at 3:52 PM Harvey Yang <chenghaoyang@chromium.org> wrote:
>
> From: Xing-Gu Chen <xinggu@chromium.org>
>
> Change ipa_control_value_entry.count to uint32_t because the
> element count of JpegApplicationSegmentContent is bigger than
> 65536.
>
> Signed-off-by: Xing-Gu Chen <xinggu@chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> Signed-off-by: Harvey Yang <chenghaoyang@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");
>
>  /**
> --
> 2.47.0.rc1.288.g06298d1525-goog
>

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@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.

> Signed-off-by: Xing-Gu Chen <xinggu@chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> Signed-off-by: Harvey Yang <chenghaoyang@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");
>  
>  /**

Hi Laurent,

On Fri, Oct 18, 2024 at 8:50 PM Laurent Pinchart
<laurent.pinchart@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@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@chromium.org>
> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > Signed-off-by: Harvey Yang <chenghaoyang@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