[libcamera-devel] [PATCH v3 03/19] ipa: ipu3: Document the IPAIPU3 class
Jean-Michel Hautbois
jeanmichel.hautbois at ideasonboard.com
Tue Oct 26 07:49:19 CEST 2021
Hi Laurent,
On 25/10/2021 22:05, Laurent Pinchart wrote:
> Hi Jean-Michel,
>
> Thank you for the patch.
>
> On Fri, Oct 22, 2021 at 05:12:02PM +0200, Jean-Michel Hautbois wrote:
>> Clarify the roles and interactions between the pipeline handler events
>> and the algorithm calls by documenting all the remaining functions of
>> the class.
>>
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>> src/ipa/ipu3/ipu3.cpp | 83 +++++++++++++++++++++++++++++++++++++------
>> 1 file changed, 73 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index 6e28a5aa..c5461d8e 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -301,9 +301,9 @@ private:
>> struct IPAContext context_;
>> };
>>
>> -/*
>> - * Compute IPASessionConfiguration using the sensor information and the sensor
>> - * v4l2 controls.
>> +/**
>> + * \brief Compute IPASessionConfiguration using the sensor information and the
>> + * sensor v4l2 controls
>
> s/v4l2/V4L2/
>
>> */
>> void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo,
>> const ControlInfoMap &sensorControls)
>> @@ -332,9 +332,9 @@ void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo,
>> context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);
>> }
>>
>> -/*
>> - * Compute camera controls using the sensor information and the sensor
>> - * v4l2 controls.
>> +/**
>> + * \brief Compute camera controls using the sensor information and the sensor
>> + * v4l2 controls
>
> Ditto.
>
>> *
>> * Some of the camera controls are computed by the pipeline handler, some others
>> * by the IPA module which is in charge of handling, for example, the exposure
>> @@ -395,7 +395,7 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,
>> }
>>
>> /**
>> - * Initialize the IPA module and its controls.
>> + * \brief Initialize the IPA module and its controls
>> *
>> * This function receives the camera sensor information from the pipeline
>> * handler, computes the limits of the controls it handles and returns
>> @@ -426,8 +426,15 @@ int IPAIPU3::init(const IPASettings &settings,
>> return 0;
>> }
>>
>> +/**
>> + * \brief Perform any processing required before the first frame
>> + */
>> int IPAIPU3::start()
>> {
>> + /*
>> + * Set the sensors V4L2 controls before the first frame to ensure that
>> + * we have an expected and known configuration from the start.
>> + */
>> setControls(0);
>>
>> return 0;
>> @@ -581,6 +588,10 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>> return 0;
>> }
>>
>> +/**
>> + * \brief Map the parameters and stats buffers allocated in the Pipeline Handler
>> + * \param[in] buffers The buffers to map
>> + */
>> void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
>> {
>> for (const IPABuffer &buffer : buffers) {
>> @@ -590,6 +601,10 @@ void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
>> }
>> }
>>
>> +/**
>> + * \brief Unmap the parameters and stats buffers
>> + * \param[in] ids The IDs of the buffers to unmap
>> + */
>> void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids)
>> {
>> for (unsigned int id : ids) {
>> @@ -601,6 +616,10 @@ void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids)
>> }
>> }
>>
>> +/**
>> + * \brief Process an event generated by the Pipeline Handler
>> + * \param[in] event The event sent from Pipeline Handler
>> + */
>> void IPAIPU3::processEvent(const IPU3Event &event)
>> {
>> switch (event.op) {
>> @@ -642,12 +661,26 @@ void IPAIPU3::processEvent(const IPU3Event &event)
>> }
>> }
>>
>> +/**
>> + * \brief Process a control list for a request from the application
>> + *
>> + * Parse the request to handle any IPA managed controls that were set from the
>
> My comments on v1 for the next 4 functions seem to have been ignored.
> I'll review v4.
quoting you on v1 (sorry to have missed it):
"Is it the current frame number or the next frame number ? We'll
probably have to document "current" and "next" at some point, when we'll
match per-frame controls with the corresponding frame."
It is the next frame that will be received on cio2 side, and fillParams
and parseStatistics will have the current 'output' frame number.
The logs look like:
[11818] DEBUG IPAIPU3 ipu3.cpp:560 processControls: frame 0
[11818] DEBUG IPAIPU3 ipu3.cpp:560 processControls: frame 1
[11818] DEBUG IPAIPU3 ipu3.cpp:560 processControls: frame 2
[11818] DEBUG IPAIPU3 ipu3.cpp:560 processControls: frame 3
[11818] DEBUG IPAIPU3 ipu3.cpp:573 fillParams: frame 0
[11818] DEBUG IPAIPU3 ipu3.cpp:608 parseStatistics: frame 0
[11818] DEBUG IPAIPU3 ipu3.cpp:560 processControls: frame 4
[11818] DEBUG IPAIPU3 ipu3.cpp:573 fillParams: frame 1
[11818] DEBUG IPAIPU3 ipu3.cpp:608 parseStatistics: frame 1
etc.
This is where the IPAFrameContext could be useful probably if it was a
queue and not the latest frame only, as we could have the
delayedControls received for frame 3 when we are processing frame 3.
This is where I am right now with per-frame controls btw, I can't see
another way to do it correctly :-).
Which means that for this function, frame is the next frame to be
processed, for parseStatistics it is the latest frame processed, and for
fillParams... Well technically it is also the latest frame processed but
it applies to the next frame... ?
>
>> + * application such as manual sensor settings.
>> + */
>> void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
>> [[maybe_unused]] const ControlList &controls)
>> {
>> /* \todo Start processing for 'frame' based on 'controls'. */
>> }
>>
>> +/**
>> + * \brief Fill the ImgU parameter buffer for the next frame
>> + * \param[in] frame The current frame number
>> + * \param[inout] params the parameter buffer to update
>> + *
>> + * Algorithms are expected to fill the IPU3 parameter buffer for the next
>> + * frame given their most recent processing of the ImgU statistics.
>> + */
>> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>> {
>> /*
>> @@ -670,6 +703,16 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>> queueFrameAction.emit(frame, op);
>> }
>>
>> +/**
>> + * \brief Process the statistics generated by the ImgU
>> + * \param[in] frame The current frame number
>> + * \param[in] frameTimestamp The current frame timestamp
>> + * \param[in] stats The IPU3 statistics and ISP results
>> + *
>> + * Parse the most recently processed image statistics from the ImgU. The
>> + * statistics are passed to each algorithm module to run their calculations and
>> + * update their state accordingly.
>> + */
>> void IPAIPU3::parseStatistics(unsigned int frame,
>> [[maybe_unused]] int64_t frameTimestamp,
>> [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
>> @@ -693,6 +736,13 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>> queueFrameAction.emit(frame, op);
>> }
>>
>> +/**
>> + * \brief Handle V4L2 controls for a given \a frame number
>> + * \param[in] frame The frame on which the V4L2 controls should be set
>> + *
>> + * Send the desired sensor control values to the Pipeline handler to request
>> + * that they are applied on the Camera sensor.
>> + */
>> void IPAIPU3::setControls(unsigned int frame)
>> {
>> IPU3Action op;
>> @@ -711,10 +761,15 @@ void IPAIPU3::setControls(unsigned int frame)
>>
>> } /* namespace ipa::ipu3 */
>>
>> -/*
>> - * External IPA module interface
>> +/**
>> + * \brief External IPA module interface
>> + *
>> + * The IPAModuleInfo is required to match an IPA module construction against the
>> + * intented pipeline handler with the module. The API and Pipeline handler
>> + * versions must match the corresponding IPA interface and pipeline handler.
>> + *
>> + * \sa struct IPAModuleInfo
>> */
>> -
>> extern "C" {
>> const struct IPAModuleInfo ipaModuleInfo = {
>> IPA_MODULE_API_VERSION,
>> @@ -723,6 +778,14 @@ const struct IPAModuleInfo ipaModuleInfo = {
>> "ipu3",
>> };
>>
>> +/**
>> + * \brief Create an instance of the IPA interface
>> + *
>> + * This function is the entry point of the IPA module. It is called by the IPA
>> + * manager to create an instance of the IPA interface for each camera. When
>> + * matched against with a pipeline handler, the IPAManager will construct an IPA
>> + * instance for each associated Camera.
>> + */
>> IPAInterface *ipaCreate()
>> {
>> return new ipa::ipu3::IPAIPU3();
>
More information about the libcamera-devel
mailing list