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

[Kieran Bingham]

Quoting Laurent Pinchart (2021-11-23 16:59:29)
> On Tue, Nov 23, 2021 at 03:34:48PM +0000, 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__
> 
> Can I volunteer someone to switch all our headers to "#pragma once" ?

I'd love to ... Can we do that? I thought I'd brought that up before and
for some reason we couldn't ...


Get ready for a patch storm...

Ok - so ... not straight away ...


> 
> > > +#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?
> > 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)
> 
> -- 
> Regards,
> 
> Laurent Pinchart