[libcamera-devel] [PATCH v3 07/11] ipa: rkisp1: Use the Algorithm class

[Kieran Bingham]

Quoting Jean-Michel Hautbois (2021-11-23 15:37:41)
> 
> 
> On 23/11/2021 16:34, Kieran Bingham wrote:
> > Quoting Jean-Michel Hautbois (2021-11-23 15:04:19)
> >> Now that libipa offers a templated class for Algorithm, use it in
> >> RkISP1.
> >>
> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> >> ---
> >>   src/ipa/rkisp1/algorithms/algorithm.h | 28 +++++++++++++++++++++++++++
> >>   src/ipa/rkisp1/algorithms/meson.build |  4 ++++
> >>   src/ipa/rkisp1/meson.build            |  4 ++++
> >>   src/ipa/rkisp1/rkisp1.cpp             |  5 ++++-
> >>   4 files changed, 40 insertions(+), 1 deletion(-)
> >>   create mode 100644 src/ipa/rkisp1/algorithms/algorithm.h
> >>   create mode 100644 src/ipa/rkisp1/algorithms/meson.build
> >>
> >> diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h
> >> new file mode 100644
> >> index 00000000..dfa58727
> >> --- /dev/null
> >> +++ b/src/ipa/rkisp1/algorithms/algorithm.h
> >> @@ -0,0 +1,28 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2021, Ideas On Board
> >> + *
> >> + * algorithm.h - RkISP1 control algorithm interface
> >> + */
> >> +#ifndef __LIBCAMERA_IPA_RKISP1_ALGORITHM_H__
> > 
> > We're under ipa/rkisp1/algorithms/algorithm.h
> > So with the current style, that might need an extra _ALGORITHM_
> > 
> > But it also feels a bit redundant to have
> > ..._ALGORITHM_ALGORITHM_H__
> 
> I really dislike this :-) ! And in IPU3 we have 
> __LIBCAMERA_IPA_IPU3_ALGORITHM_H__

Yes, me too - so lets leave it as it is.


> 
> > 
> >> +#define __LIBCAMERA_IPA_RKISP1_ALGORITHM_H__
> >> +
> >> +#include <linux/rkisp1-config.h>
> >> +
> >> +#include <libcamera/ipa/rkisp1_ipa_interface.h>
> >> +
> >> +#include <libipa/algorithm.h>
> >> +
> >> +#include "ipa_context.h"
> >> +
> >> +namespace libcamera {
> >> +
> >> +namespace ipa::rkisp1 {
> >> +
> >> +using Algorithm = libcamera::ipa::Algorithm<IPAContext, IPACameraSensorInfo, rkisp1_params_cfg, rkisp1_stat_buffer>;
> >> +
> >> +} /* namespace ipa::rkisp1 */
> >> +
> >> +} /* namespace libcamera */
> >> +
> >> +#endif /* __LIBCAMERA_IPA_RKISP1_ALGORITHM_H__ */
> >> diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
> >> new file mode 100644
> >> index 00000000..1c6c59cf
> >> --- /dev/null
> >> +++ b/src/ipa/rkisp1/algorithms/meson.build
> >> @@ -0,0 +1,4 @@
> >> +# SPDX-License-Identifier: CC0-1.0
> >> +
> >> +rkisp1_ipa_algorithms = files([
> >> +])
> >> diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build
> >> index 3683c922..8c822fbb 100644
> >> --- a/src/ipa/rkisp1/meson.build
> >> +++ b/src/ipa/rkisp1/meson.build
> >> @@ -1,5 +1,7 @@
> >>   # SPDX-License-Identifier: CC0-1.0
> >>   
> >> +subdir('algorithms')
> >> +
> >>   ipa_name = 'ipa_rkisp1'
> >>   
> >>   rkisp1_ipa_sources = files([
> >> @@ -7,6 +9,8 @@ rkisp1_ipa_sources = files([
> >>       'rkisp1.cpp',
> >>   ])
> >>   
> >> +rkisp1_ipa_sources += rkisp1_ipa_algorithms
> >> +
> >>   mod = shared_module(ipa_name,
> >>                       [rkisp1_ipa_sources, libcamera_generated_ipa_headers],
> >>                       name_prefix : '',
> >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> >> index 34c3f9a2..0c54d8ec 100644
> >> --- a/src/ipa/rkisp1/rkisp1.cpp
> >> +++ b/src/ipa/rkisp1/rkisp1.cpp
> >> @@ -25,7 +25,7 @@
> >>   
> >>   #include <libcamera/internal/mapped_framebuffer.h>
> >>   
> >> -#include "ipa_context.h"
> > 
> > Is this intentionally removed?
> 
> Yes, because it is now included by algorithm.h wchich need it to define 
> its template parameter Context.
> 

I'd be tempted to say it should still be specified here, as we shouldn't
rely on other component headers to provide the headers we need.

But equally we can't anticipate the algorithm.h to remove the context.h
as that's fundamental property of the algorithms.

IWYU [0] would complain, but either way, it won't bother me so much,
and my tag is already there.

[0] https://include-what-you-use.org/

> > I suspect it should be put after the libipa/camera_sensor_helper.h...
> > 
> > Which means libipa/camera_sensor_helper.h should probably have been put
> > before ipa_context.h in the patch that added that...
> > 
> > With the minors resolved:
> > 
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > 
> >> +#include "algorithms/algorithm.h"
> >>   #include "libipa/camera_sensor_helper.h"
> >>   
> >>   namespace libcamera {
> >> @@ -82,6 +82,9 @@ private:
> >>   
> >>          /* Local parameter storage */
> >>          struct IPAContext context_;
> >> +
> >> +       /* Maintain the algorithms used by the IPA */
> >> +       std::list<std::unique_ptr<ipa::rkisp1::Algorithm>> algorithms_;
> >>   };
> >>   
> >>   int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)
> >> -- 
> >> 2.32.0
> >>