[libcamera-devel] [PATCH v3 IPAIPU3 2/2] ipu3: Use the new IPAIPU3 interface

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Mon, Mar 28, 2022 at 11:06:12PM +0530, Umang Jain via libcamera-devel wrote:
> Use the new IPAIPU3 interface that migrates the event-based ops
> to dedication functions.
> 
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  ipu3.cpp | 108 +++++++++++++++++++++++--------------------------------
>  1 file changed, 45 insertions(+), 63 deletions(-)
> 
> diff --git a/ipu3.cpp b/ipu3.cpp
> index 6ab86e0..d02581d 100644
> --- a/ipu3.cpp
> +++ b/ipu3.cpp
> @@ -49,13 +49,16 @@ public:
>  
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> -	void processEvent(const IPU3Event &event) override;
> +
> +	void processParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;
> +	void processControls(const uint32_t frame, const ControlList &controls) override;

Same comment as in 1/2 regarding the name of those two functions.

> +	void processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,
> +				const uint32_t bufferId, const ControlList &sensorControls) override;
>  
>  private:
>  	void updateControls(const IPACameraSensorInfo &sensorInfo,
>  			    const ControlInfoMap &sensorControls,
>  			    ControlInfoMap *ipaControls);
> -	void processControls(unsigned int frame, const ControlList &metadata);
>  	void runAiq(unsigned int frame);
>  	void fillParams(unsigned int frame, ipu3_uapi_params *params);
>  	void parseStatistics(unsigned int frame,
> @@ -321,54 +324,41 @@ void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids)
>  	}
>  }
>  
> -void IPAIPU3::processEvent(const IPU3Event &event)
> +void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
> +			      [[maybe_unused]] const ControlList &controls)
>  {
> -	switch (event.op) {
> -	case EventProcessControls: {
> -		processControls(event.frame, event.controls);
> -		break;
> -	}
> -	case EventStatReady: {
> -		auto it = buffers_.find(event.bufferId);
> -		if (it == buffers_.end()) {
> -			LOG(IPAIPU3, Error) << "Could not find stats buffer!";
> -			return;
> -		}
> -
> -		Span<uint8_t> mem = it->second.maps()[0];
> -		const ipu3_uapi_stats_3a *stats =
> -			reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
> -
> -		parseStatistics(event.frame,
> -				event.frameTimestamp,
> -				stats,
> -				event.sensorControls);
> -		break;
> -	}
> -	case EventFillParams: {
> -		auto it = buffers_.find(event.bufferId);
> -		if (it == buffers_.end()) {
> -			LOG(IPAIPU3, Error) << "Could not find param buffer!";
> -			return;
> -		}
> -
> -		Span<uint8_t> mem = it->second.maps()[0];
> -		ipu3_uapi_params *params =
> -			reinterpret_cast<ipu3_uapi_params *>(mem.data());
> -
> -		fillParams(event.frame, params);
> -		break;
> -	}
> -	default:
> -		LOG(IPAIPU3, Error) << "Unknown event " << event.op;
> -		break;
> +	/* \todo Start processing for 'frame' based on 'controls'. */
> +}
> +
> +void IPAIPU3::processParamsBuffer(const uint32_t frame, const uint32_t bufferId)
> +{
> +	auto it = buffers_.find(bufferId);
> +	if (it == buffers_.end()) {
> +		LOG(IPAIPU3, Error) << "Could not find params buffer";
> +		return;
>  	}
> +
> +	Span<uint8_t> mem = it->second.maps()[0];
> +	ipu3_uapi_params *params =
> +		reinterpret_cast<ipu3_uapi_params *>(mem.data());
> +
> +	fillParams(frame, params);
>  }
>  
> -void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
> -			      [[maybe_unused]] const ControlList &controls)
> +void IPAIPU3::processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,
> +				 const uint32_t bufferId, const ControlList &sensorControls)
>  {
> -	/* \todo Start processing for 'frame' based on 'controls'. */
> +	auto it = buffers_.find(bufferId);
> +	if (it == buffers_.end()) {
> +		LOG(IPAIPU3, Error) << "Could not find stats buffer";
> +		return;
> +	}
> +
> +	Span<uint8_t> mem = it->second.maps()[0];
> +	const ipu3_uapi_stats_3a *stats =
> +		reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
> +
> +	parseStatistics(frame, frameTimestamp, stats, sensorControls);
>  }
>  
>  void IPAIPU3::runAiq([[maybe_unused]] unsigned int frame)
> @@ -424,10 +414,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>  
>  	setControls(frame);
>  
> -	IPU3Action op;
> -	op.op = ActionParamFilled;
> -
> -	queueFrameAction.emit(frame, op);
> +	paramsBufferReady.emit(frame);
>  }
>  
>  void IPAIPU3::parseStatistics(unsigned int frame,
> @@ -488,27 +475,22 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>  				(sensorInfo_.pixelRate / 1e6);
>  	ctrls.set(controls::FrameDuration, frameDuration);
>  
> -	IPU3Action op;
> -	op.op = ActionMetadataReady;
> -	op.controls = ctrls;
> -
> -	queueFrameAction.emit(frame, op);
> +	metadataReady.emit(frame, ctrls);
>  }
>  
>  void IPAIPU3::setControls(unsigned int frame)
>  {
> -	IPU3Action op;
> -	op.op = ActionSetSensorControls;
> -
> -	ControlList sensorCtrls(ctrls_);
> -	sensorCtrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
> -	sensorCtrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));
> +	ControlList ctrls(ctrls_);
> +	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
> +	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));

How about keeping the name sensorCtrls to differentiate it from
lensCtrls ?

>  
> -	op.sensorControls = sensorCtrls;
> +	ControlList lensCtrls(ctrls_);
> +	/*
> +	 * \todo: lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, <lens_position>));
> +	 */

This doesn't look right.

>  
> -	op.lensControls.set(V4L2_CID_FOCUS_ABSOLUTE, lensPosition_);
>  
> -	queueFrameAction.emit(frame, op);
> +	setSensorControls.emit(frame, ctrls, lensCtrls);
>  }
>  
>  } /* namespace ipa::ipu3 */

-- 
Regards,

Laurent Pinchart