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

[Jean-Michel Hautbois]

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__

> 
>> +#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 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
>>