[libcamera-devel] [PATCH] ipa: ipu3: Introduce IPAConfigInfo in IPC
Umang Jain
umang.jain at ideasonboard.com
Tue Apr 27 08:10:25 CEST 2021
Hi Laurent,
Thanks for the reviews.
On 4/27/21 8:27 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Mon, Apr 26, 2021 at 10:46:08PM +0530, Umang Jain wrote:
>> IPAConfigInfo is a consolidated data structure passed from IPU3
>> pipeline-handler to IPU3 IPA. This provides a streamlined and
>> extensible way to provide the configuration data to IPA interface.
> The commit message should mention that you're adding more data to the
> structure, and why.
>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> ---
>> include/libcamera/ipa/ipu3.mojom | 11 +++++++++--
>> src/ipa/ipu3/ipu3.cpp | 14 ++++++--------
>> src/libcamera/pipeline/ipu3/ipu3.cpp | 7 ++++++-
>> 3 files changed, 21 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
>> index a717b1e6..88cbb403 100644
>> --- a/include/libcamera/ipa/ipu3.mojom
>> +++ b/include/libcamera/ipa/ipu3.mojom
>> @@ -25,13 +25,20 @@ struct IPU3Action {
>> libcamera.ControlList controls;
>> };
>>
>> +struct IPAConfigInfo {
>> + libcamera.CameraSensorInfo sensorInfo;
>> + map<uint32, ControlInfoMap> entityControls;
>> + libcamera.Size bdsOutputSize;
>> + libcamera.Size iif;
>> + libcamera.Size cropRegion;
>> +};
> Could you document this structure with doxygen comments ? We don't
> generate documentation from mojom files yet, but we should still be
> prepared.
>
>> +
>> interface IPAIPU3Interface {
>> init(libcamera.IPASettings settings) => (int32 ret);
>> start() => (int32 ret);
>> stop();
>>
>> - configure(map<uint32, libcamera.ControlInfoMap> entityControls,
>> - libcamera.Size bdsOutputSize) => ();
>> + configure(IPAConfigInfo configInfo) => ();
> While at it, I think you can drop the => ().
>
>> mapBuffers(array<libcamera.IPABuffer> buffers);
>> unmapBuffers(array<uint32> ids);
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index f5343547..a77afd55 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -43,8 +43,7 @@ public:
>> int start() override;
>> void stop() override {}
>>
>> - void configure(const std::map<uint32_t, ControlInfoMap> &entityControls,
>> - const Size &bdsOutputSize) override;
>> + void configure(const struct IPAConfigInfo &configInfo) override;
>>
>> void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>> void unmapBuffers(const std::vector<unsigned int> &ids) override;
>> @@ -139,13 +138,12 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)
>> << (int)bdsGrid_.height << " << " << (int)bdsGrid_.block_height_log2 << ")";
>> }
>>
>> -void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls,
>> - const Size &bdsOutputSize)
>> +void IPAIPU3::configure(const struct IPAConfigInfo &configInfo)
>> {
>> - if (entityControls.empty())
>> + if (configInfo.entityControls.empty())
>> return;
>>
>> - ctrls_ = entityControls.at(0);
>> + ctrls_ = configInfo.entityControls.at(0);
>>
>> const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
>> if (itExp == ctrls_.end()) {
>> @@ -169,10 +167,10 @@ void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls
>>
>> params_ = {};
>>
>> - calculateBdsGrid(bdsOutputSize);
>> + calculateBdsGrid(configInfo.bdsOutputSize);
>>
>> awbAlgo_ = std::make_unique<IPU3Awb>();
>> - awbAlgo_->initialise(params_, bdsOutputSize, bdsGrid_);
>> + awbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);
>>
>> agcAlgo_ = std::make_unique<IPU3Agc>();
>> agcAlgo_->initialise(bdsGrid_);
>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> index 73306cea..be43d5a1 100644
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> @@ -635,7 +635,12 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>>
>> std::map<uint32_t, ControlInfoMap> entityControls;
>> entityControls.emplace(0, data->cio2_.sensor()->controls());
>> - data->ipa_->configure(entityControls, config->imguConfig().bds);
>> +
>> + struct ipa::ipu3::IPAConfigInfo configInfo = {
> You can drop the struct keyword.
>
>> + sensorInfo, entityControls, config->imguConfig().bds,
>> + config->imguConfig().iif, data->cropRegion_.size()
>> + };
> Given that several members of this structures have the same type, it
> would be safer to use named initializers.
>
> ipa::ipu3::IPAConfigInfo configInfo = {
> .sensorInfo = sensorInfo,
> .entityControls = entityControls,
> .bdsOutputSize = config->imguConfig().bds,
> .iif = config->imguConfig().iif,
> .cropRegion = data->cropRegion_.size(),
> };
This throws a compile-time error because the generated
ipu3-ipa-interface.h code doesn't suppport named initializers for
IPAConfigInfo I think. I think we need to support in mojom first?
FAILED: src/libcamera/4ab8042@@camera at sha/pipeline_ipu3_ipu3.cpp.o
../../../../../tmp/portage/media-libs/libcamera-9999/work/libcamera-9999/src/libcamera/pipeline/ipu3/ipu3.cpp:639:27:
error: no matching constructor for initialization of
'ipa::ipu3::IPAConfigInfo'
ipa::ipu3::IPAConfigInfo configInfo = {
^ ~
include/libcamera/ipa/ipu3_ipa_interface.h:96:2: note: candidate
constructor not viable: cannot convert argument of incomplete type
'void' to 'const libcamera::CameraSensorInfo' for 1st argument
IPAConfigInfo(const CameraSensorInfo &_sensorInfo, const
std::map<uint32_t, ControlInfoMap> &_entityControls, const Size
&_bdsOutputSize, const Size &_iif)
^
include/libcamera/ipa/ipu3_ipa_interface.h:89:8: note: candidate
constructor (the implicit copy constructor) not viable: requires 1
argument, but 4 were provided
struct IPAConfigInfo
^
include/libcamera/ipa/ipu3_ipa_interface.h:89:8: note: candidate
constructor (the implicit move constructor) not viable: requires 1
argument, but 4 were provided
include/libcamera/ipa/ipu3_ipa_interface.h:92:2: note: candidate
constructor not viable: requires 0 arguments, but 4 were provided
IPAConfigInfo()
^
1 error generated.
Meanwhile, in order to address your concerns, maybe this?
+ ipa::ipu3::IPAConfigInfo configInfo;
+ configInfo.sensorInfo = sensorInfo;
+ configInfo.entityControls = entityControls;
+ configInfo.bdsOutputSize = config->imguConfig().bds;
+ configInfo.iif = config->imguConfig().iif;
>> + data->ipa_->configure(configInfo);
>>
>> return 0;
>> }
More information about the libcamera-devel
mailing list