[PATCH] libcamera: rkisp1: Rationalize IPA and handlers names

[Milan Zamazal]

Jacopo Mondi <jacopo.mondi at ideasonboard.com> writes:

> Hi Milan
>   thanks for reviw
>
> On Tue, Oct 29, 2024 at 10:28:22AM +0100, Milan Zamazal wrote:
>> Milan Zamazal <mzamazal at redhat.com> writes:
>>
>> > Hi Jacopo,
>> >
>> > thank you for the patch.  As somebody who came to libcamera and had to
>> > understand that stuff, I can confirm the original terminology is quite
>> > confusing and your patch is a good improvement.
>>
>> (And now the question is who would make similar changes to the other
>> pipelines; I can take care of `simple'.)
>>
>
> Well, the first one who gets there and is annoyed by the existing
> names I would say :)
>
> I only count vimc and ipu3, soft-isp apart.
>
> Let's start with this and simple if you like, then we'll get to the
> other ones eventually ? 

I'd say this would be generally easier.

> After all, the IPA interface names doesn't need to be standardized
> across all platforms.
>
> On the other hand, true, for consistency I should rather bite the
> bullet and convert all of them in one go ?
>
>> > Jacopo Mondi <jacopo.mondi at ideasonboard.com> writes:
>> >
>> >> The names used by the IPA interface and the names used for buffer
>> >> completions handlers for the RkISP1 clash in the use of the term "buffer".
>> >>
>> >> For example the video device buffer completion handler is called
>> >> "bufferReady" and the IPA event to ask the IPA to compute parameters is
>> >> called "fillParamsBuffers". This makes it hard to recognize which
>> >> function handles video device completion signals and which ones handle
>> >> the IPA interface events.
>> >>
>> >> Rationalize the naming scheme in the IPA interface function and events
>> >> and the signal handlers in the RkISP1 support, according to the
>> >> following table. Remove the name "buffer" from the IPA interface events
>> >> and events handler and reserve it for the buffer completion handlers.
>> >> Rename the IPA interface events and function to use the 'params' and
>> >> 'stats' names and make the signal and the even names specular (ie
>> >> 'computeParams' : 'paramsComputed')
>> >>
>> >> IPA Interface:
>> >>
>> >> - fillParamsBuffer -> computeParams   [FUNCTION]
>> >> - paramsBufferReady -> paramsComputed [EVENT]
>> >>
>> >> - processStatsBuffer -> processStats  [FUNCTION]
>> >> - metadataReady -> statsProcessed     [EVENT]
>> >>
>> >> Pipeline handler:
>> >>
>> >> - bufferReady -> videoBufferReady     [BUFFER HANDLER]
>> >> - paramReady -> paramsBufferReady     [BUFFER HANDLER]
>> >> - statReady -> statsBufferReady       [BUFFER HANDLER]
>> >> - paramFilled -> paramsComputed       [IPA EVENT HANDLER]
>> >> - metadataReady -> statsProcessed     [IPA EVENT HANDLER]
>> >>
>> >> Cosmetic change only, no functional changes intended.
>> >>
>> >> Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>> >
>> > Reviewed-by: Milan Zamazal <mzamazal at redhat.com>
>> >
>> >> ---
>> >>  include/libcamera/ipa/rkisp1.mojom       | 10 +++---
>> >>  src/ipa/rkisp1/rkisp1.cpp                | 16 ++++-----
>> >>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 42 ++++++++++++------------
>> >>  3 files changed, 34 insertions(+), 34 deletions(-)
>> >>
>> >> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
>> >> index 80d54a03aa90..ced1526e18cd 100644
>> >> --- a/include/libcamera/ipa/rkisp1.mojom
>> >> +++ b/include/libcamera/ipa/rkisp1.mojom
>> >> @@ -31,13 +31,13 @@ interface IPARkISP1Interface {
>> >>  	unmapBuffers(array<uint32> ids);
>> >>
>> >>  	[async] queueRequest(uint32 frame, libcamera.ControlList reqControls);
>> >> -	[async] fillParamsBuffer(uint32 frame, uint32 bufferId);
>> >> -	[async] processStatsBuffer(uint32 frame, uint32 bufferId,
>> >> -				   libcamera.ControlList sensorControls);
>> >> +	[async] computeParams(uint32 frame, uint32 bufferId);
>> >> +	[async] processStats(uint32 frame, uint32 bufferId,
>> >> +			     libcamera.ControlList sensorControls);
>> >>  };
>> >>
>> >>  interface IPARkISP1EventInterface {
>> >> -	paramsBufferReady(uint32 frame, uint32 bytesused);
>> >> +	paramsComputed(uint32 frame, uint32 bytesused);
>> >>  	setSensorControls(uint32 frame, libcamera.ControlList sensorControls);
>> >> -	metadataReady(uint32 frame, libcamera.ControlList metadata);
>> >> +	statsProcessed(uint32 frame, libcamera.ControlList metadata);
>> >>  };
>> >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>> >> index 9e161cabdea4..6d6f8392a70f 100644
>> >> --- a/src/ipa/rkisp1/rkisp1.cpp
>> >> +++ b/src/ipa/rkisp1/rkisp1.cpp
>> >> @@ -65,9 +65,9 @@ public:
>> >>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
>> >>
>> >>  	void queueRequest(const uint32_t frame, const ControlList &controls) override;
>> >> -	void fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;
>> >> -	void processStatsBuffer(const uint32_t frame, const uint32_t bufferId,
>> >> -				const ControlList &sensorControls) override;
>> >> +	void computeParams(const uint32_t frame, const uint32_t bufferId) override;
>> >> +	void processStats(const uint32_t frame, const uint32_t bufferId,
>> >> +			  const ControlList &sensorControls) override;
>> >>
>> >>  protected:
>> >>  	std::string logPrefix() const override;
>> >> @@ -335,7 +335,7 @@ void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls)
>> >>  	}
>> >>  }
>> >>
>> >> -void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)
>> >> +void IPARkISP1::computeParams(const uint32_t frame, const uint32_t bufferId)
>> >>  {
>> >>  	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>> >>
>> >> @@ -345,11 +345,11 @@ void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)
>> >>  	for (auto const &algo : algorithms())
>> >>  		algo->prepare(context_, frame, frameContext, &params);
>> >>
>> >> -	paramsBufferReady.emit(frame, params.size());
>> >> +	paramsComputed.emit(frame, params.size());
>> >>  }
>> >>
>> >> -void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId,
>> >> -				   const ControlList &sensorControls)
>> >> +void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId,
>> >> +			     const ControlList &sensorControls)
>> >>  {
>> >>  	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>> >>
>> >> @@ -378,7 +378,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
>> >>
>> >>  	setControls(frame);
>> >>
>> >> -	metadataReady.emit(frame, metadata);
>> >> +	statsProcessed.emit(frame, metadata);
>> >>  }
>> >>
>> >>  void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,
>> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> >> index c7b0b3927da1..83b74b27652f 100644
>> >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> >> @@ -114,11 +114,11 @@ public:
>> >>  	ControlInfoMap ipaControls_;
>> >>
>> >>  private:
>> >> -	void paramFilled(unsigned int frame, unsigned int bytesused);
>> >> +	void paramsComputed(unsigned int frame, unsigned int bytesused);
>> >>  	void setSensorControls(unsigned int frame,
>> >>  			       const ControlList &sensorControls);
>> >>
>> >> -	void metadataReady(unsigned int frame, const ControlList &metadata);
>> >> +	void statsProcessed(unsigned int frame, const ControlList &metadata);
>> >>  };
>> >>
>> >>  class RkISP1CameraConfiguration : public CameraConfiguration
>> >> @@ -180,9 +180,9 @@ private:
>> >>  		      const RkISP1CameraConfiguration &config);
>> >>  	int createCamera(MediaEntity *sensor);
>> >>  	void tryCompleteRequest(RkISP1FrameInfo *info);
>> >> -	void bufferReady(FrameBuffer *buffer);
>> >> -	void paramReady(FrameBuffer *buffer);
>> >> -	void statReady(FrameBuffer *buffer);
>> >> +	void videoBufferReady(FrameBuffer *buffer);
>> >> +	void paramsBufferReady(FrameBuffer *buffer);
>> >> +	void statsBufferReady(FrameBuffer *buffer);
>> >>  	void dewarpBufferReady(FrameBuffer *buffer);
>> >>  	void frameStart(uint32_t sequence);
>> >>
>> >> @@ -367,8 +367,8 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
>> >>  		return -ENOENT;
>> >>
>> >>  	ipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls);
>> >> -	ipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled);
>> >> -	ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady);
>> >> +	ipa_->paramsComputed.connect(this, &RkISP1CameraData::paramsComputed);
>> >> +	ipa_->statsProcessed.connect(this, &RkISP1CameraData::statsProcessed);
>> >>
>> >>  	/*
>> >>  	 * The API tuning file is made from the sensor name unless the
>> >> @@ -400,7 +400,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
>> >>  	return 0;
>> >>  }
>> >>
>> >> -void RkISP1CameraData::paramFilled(unsigned int frame, unsigned int bytesused)
>> >> +void RkISP1CameraData::paramsComputed(unsigned int frame, unsigned int bytesused)
>> >>  {
>> >>  	PipelineHandlerRkISP1 *pipe = RkISP1CameraData::pipe();
>> >>  	RkISP1FrameInfo *info = frameInfo_.find(frame);
>> >> @@ -424,7 +424,7 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,
>> >>  	delayedCtrls_->push(sensorControls);
>> >>  }
>> >>
>> >> -void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)
>> >> +void RkISP1CameraData::statsProcessed(unsigned int frame, const ControlList &metadata)
>> >>  {
>> >>  	RkISP1FrameInfo *info = frameInfo_.find(frame);
>> >>  	if (!info)
>> >> @@ -1120,8 +1120,8 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
>> >>  		if (data->selfPath_ && info->selfPathBuffer)
>> >>  			data->selfPath_->queueBuffer(info->selfPathBuffer);
>> >>  	} else {
>> >> -		data->ipa_->fillParamsBuffer(data->frame_,
>> >> -					     info->paramBuffer->cookie());
>> >> +		data->ipa_->computeParams(data->frame_,
>> >> +					  info->paramBuffer->cookie());
>> >>  	}
>> >>
>> >>  	data->frame_++;
>> >> @@ -1334,11 +1334,11 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>> >>  	if (hasSelfPath_ && !selfPath_.init(media_))
>> >>  		return false;
>> >>
>> >> -	mainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);
>> >> +	mainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::videoBufferReady);
>> >>  	if (hasSelfPath_)
>> >> -		selfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);
>> >> -	stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);
>> >> -	param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);
>> >> +		selfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::videoBufferReady);
>> >> +	stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statsBufferReady);
>> >> +	param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramsBufferReady);
>> >>
>> >>  	/* If dewarper is present, create its instance. */
>> >>  	DeviceMatch dwp("dw100");
>> >> @@ -1399,7 +1399,7 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)
>> >>  	completeRequest(request);
>> >>  }
>> >>
>> >> -void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
>> >> +void PipelineHandlerRkISP1::videoBufferReady(FrameBuffer *buffer)
>> >>  {
>> >>  	ASSERT(activeCamera_);
>> >>  	RkISP1CameraData *data = cameraData(activeCamera_);
>> >> @@ -1424,7 +1424,7 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
>> >>  		if (isRaw_) {
>> >>  			const ControlList &ctrls =
>> >>  				data->delayedCtrls_->get(metadata.sequence);
>> >> -			data->ipa_->processStatsBuffer(info->frame, 0, ctrls);
>> >> +			data->ipa_->processStats(info->frame, 0, ctrls);
>> >>  		}
>> >>  	} else {
>> >>  		if (isRaw_)
>> >> @@ -1508,7 +1508,7 @@ void PipelineHandlerRkISP1::dewarpBufferReady(FrameBuffer *buffer)
>> >>  	tryCompleteRequest(info);
>> >>  }
>> >>
>> >> -void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)
>> >> +void PipelineHandlerRkISP1::paramsBufferReady(FrameBuffer *buffer)
>> >>  {
>> >>  	ASSERT(activeCamera_);
>> >>  	RkISP1CameraData *data = cameraData(activeCamera_);
>> >> @@ -1521,7 +1521,7 @@ void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)
>> >>  	tryCompleteRequest(info);
>> >>  }
>> >>
>> >> -void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
>> >> +void PipelineHandlerRkISP1::statsBufferReady(FrameBuffer *buffer)
>> >>  {
>> >>  	ASSERT(activeCamera_);
>> >>  	RkISP1CameraData *data = cameraData(activeCamera_);
>> >> @@ -1539,8 +1539,8 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
>> >>  	if (data->frame_ <= buffer->metadata().sequence)
>> >>  		data->frame_ = buffer->metadata().sequence + 1;
>> >>
>> >> -	data->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),
>> >> -				       data->delayedCtrls_->get(buffer->metadata().sequence));
>> >> +	data->ipa_->processStats(info->frame, info->statBuffer->cookie(),
>> >> +				 data->delayedCtrls_->get(buffer->metadata().sequence));
>> >>  }
>> >>
>> >>  REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1, "rkisp1")
>>