[libcamera-devel] [RFC PATCH v2] ipa: ipu3: Introduce IPAConfigInfo in IPC
Umang Jain
umang.jain at ideasonboard.com
Tue Apr 27 14:54:08 CEST 2021
Hi JM
On 4/27/21 5:23 PM, Jean-Michel Hautbois wrote:
> Hi Umang,
>
> Thanks for the patch !
>
> On 27/04/2021 08:35, 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 IPA interface should be common to both closed-source and
>> open-source IPU3 IPAs. Hence, the structure encompasses additional
>> configuration information required to init and configure the
>> closed-source IPA as well.
>>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> ---
>> Changes in v2:
>> - Mark as RFC(do not merge) as this is built upon Paul's
>> [Fix support for core.mojom structs v3] - waiting for merge to master
>> - Drop cropRegion_ from structure - this is provided from CameraSensorInfo
>> itself.
>> - Doxygen documentation of IPAConfigInfo.
>> ---
>> include/libcamera/ipa/ipu3.mojom | 46 ++++++++++++++++++++++++++--
>> src/ipa/ipu3/ipu3.cpp | 14 ++++-----
>> src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++-
>> 3 files changed, 58 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
>> index a717b1e6..acd5cfa4 100644
>> --- a/include/libcamera/ipa/ipu3.mojom
>> +++ b/include/libcamera/ipa/ipu3.mojom
>> @@ -25,13 +25,55 @@ struct IPU3Action {
>> libcamera.ControlList controls;
>> };
>>
>> +/**
>> + * \struct IPAConfigInfo
>> + * \brief Information to be passed from IPU3 pipeline-handler to IPA
>> + *
>> + * The IPAConfigInfo structure stores the data passed from IPU3 pipeline-handler
>> + * to the IPAIPU3Interface::configure(). The structure provides extensibility
>> + * for additional configuration data as required, for closed-source or
>> + * open-source IPAs' configure() phases.
>> + */
>> +
>> +/**
>> + * \var IPAConfigInfo::sensorInfo
>> + * \brief Camera sensor information
>> + *
>> + * Provides the camera sensor information such as model name, pixel-rate,
>> + * output-size and so on. Refer to libcamera::CameraSensorInfo for further
>> + * details.
>> + */
>> +
>> + /**
>> + * \var IPAConfigInfo::entityControls
>> + * \brief Controls supported by the sensor
>> + *
>> + * A map of V4L2 controls supported by the sensor in order to read or write
>> + * control values to them.
>> + */
>> +
>> + /**
>> + * \var IPAConfigInfo::bdsOutputSize
>> + * \brief Size of ImgU BDS rectangle
>> + */
>> +
>> + /**
>> + * \var IPAConfigInfo::iif
>> + * \brief Size of ImgU input-feeder rectangle
>> + */
>> +struct IPAConfigInfo {
>> + libcamera.CameraSensorInfo sensorInfo;
>> + map<uint32, ControlInfoMap> entityControls;
>> + libcamera.Size bdsOutputSize;
>> + libcamera.Size iif;
>> +};
>> +
> I think it is interesting, and would like your advice: I would need this
> same structure for rkisp1 platform, except that the sizes are obviously
> not the same.
> And so, I am wondering if we could imagine a more general structure like:
> struct IPAConfigInfo {
> libcamera.CameraSensorInfo sensorInfo;
> map<uint32, ControlInfoMap> entityControls;
> libcamera.Size ispInputSize;
> libcamera.Size ispOutputSize;
> };
>
> names and all may have to be reworked but that is the base idea.
> There is no bds in rkisp1 (afaik) but on both IPU3 and RkIsp1 at least
> you have an input an output pad size for the ISP node.
Certainly, this might be a good idea. I am not opposed to it.
The question here is, that is the structure core.mojom worthy, in order
to be shared between multiple IPAs right? If not, I think we need to
keep them in <platform IPA>.mojom for now.
And if we want to share the data-structures, we need to work out how all
existing IPAs (for e.g. Raspberry Pi) can be ported to used that
IPAConfig structure too. We probably, should address this by looking at
all the common parameters we need for all the existing IPAs and have a
patch series that ports every IPA to use that data-structure.
However, there are some risky? situations that can happen, for e.g.
- An IPA X requiring a parameter Y for configuration and the common
struct IPAConfig doesn't have a placeholder
- Parameter Y is added to common struct IPAConfig
- Parameter Y is unused/nullptr for all IPAs except IPA X
I do not know if adding placeholder for Y is fine in the common struct,
if fine or not, in the codebase purview.
Thinking about it, the chances of various IPA, having exact
configuration data (common IPAConfig) are slim. Hence, the IPA authors
will try hacks for other parameters they need for IPA configuration,
either as function-params or introducing new structures and passing
alongside with common struct IPAConfig to the IPA. That would not be
nice, isn't it?
I might be wrong, so I shall keep an open mind about it. Maybe we can
track this as a task somewhere too?
>
>> interface IPAIPU3Interface {
>> init(libcamera.IPASettings settings) => (int32 ret);
>> start() => (int32 ret);
>> stop();
>>
>> - configure(map<uint32, libcamera.ControlInfoMap> entityControls,
>> - libcamera.Size bdsOutputSize) => ();
>> + configure(IPAConfigInfo configInfo);
>>
>> 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..769c24d3 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 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 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..b1ff1dbe 100644
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> @@ -635,7 +635,14 @@ 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);
>> +
>> + 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