[libcamera-devel] [PATCH] libcamera: pipeline: rkisp1: configure IPA from configure method instead of start method

[Sebastian Fricke]

Hey Dafna,

Thank you for the patch.

On 12.02.2021 20:05, Dafna Hirschfeld wrote:
>Currently the call to the configure method of rkisp1 IPA
>is called upon the 'start' of the pipeline. This should be done
>in the 'configure' method instead.

I assume that the reason for this change is consistency right? I believe
the raspberry Pi has the most mature pipeline and IPA implementation and
it makes sense to have some kind of consistent pattern.

In order to find out why it is necessary to configure the IPA in
configure instead of start, I first looked into `src/cam/capture.cpp`,
where both methods are called. In between the configure and the start
call, the buffers are allocated and the initial set of requests is
created.
Additionally, I have compared both the RkISP1 and Raspberry Pi
implementation of the IPA configure method. Currently, the RkISP1 IPA
doesn't do a lot so it is difficult to decide, which parts might be added.
The Raspberry Pi currently does the following actions that are missing in
the RkISP1:
- Setup and validate the ISP controls
- Setup metadata controls
- Load device specific information from a helper
- Load the lens shading table
- Prepare the algorithms by setting the camera mode
- Load the tuning file for the sensor

What they both do is initialize the analog gain and exposure controls.
(With the difference that RkISP1 sets both to their respective minimum
and Raspberry Pi provides default values).

This means, that the different order of the IPA configure method can
only have an impact on the actions happening in between the two states
configured and start (allocation of buffers and creation of the inital
requests). So, the current advantage/disadvantage is that the controls
are reset to their minimum for the initial set of requests as well.

 From my point of view this seems to be a good preparation for the
following changes to the RkISP1 IPA, even though it currently doesn't
seem to do to much. Have I missed anything else that makes this order of
actions essential?

Besides that little analysis I have tested the patch by building it and
performing a test capture. Both works without problems.

Greetings,
Sebastian

I fixed a small typo below that existed before as well.

>
>Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld at collabora.com>
>---
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 63 ++++++++++++------------
> 1 file changed, 31 insertions(+), 32 deletions(-)
>
>diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>index e85979a7..f15efa9f 100644
>--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>@@ -607,11 +607,24 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> 		<< "ISP output pad configured with " << format.toString()
> 		<< " crop " << rect.toString();
>
>+	std::map<unsigned int, IPAStream> streamConfig;
>+
> 	for (const StreamConfiguration &cfg : *config) {
>-		if (cfg.stream() == &data->mainPathStream_)
>+		if (cfg.stream() == &data->mainPathStream_) {
> 			ret = mainPath_.configure(cfg, format);
>-		else
>+			if (data->mainPath_->isEnabled())
>+				streamConfig[0] = {
>+					.pixelFormat = cfg.pixelFormat,
>+					.size = cfg.size,
>+				};
>+		} else {
> 			ret = selfPath_.configure(cfg, format);
>+			if (data->selfPath_->isEnabled())
>+				streamConfig[1] = {
>+					.pixelFormat = cfg.pixelFormat,
>+					.size = cfg.size,
>+				};
>+		}
>
> 		if (ret)
> 			return ret;
>@@ -629,6 +642,22 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> 	if (ret)
> 		return ret;
>
>+	/* Inform IPA of stream configuration and sensor controls. */
>+	CameraSensorInfo sensorInfo = {};
>+	ret = data->sensor_->sensorInfo(&sensorInfo);
>+	if (ret) {
>+		/* \todo Turn this in an hard failure. */

s/Turn this in an hard failure./Turn this into a hard failure./
(or maybe: Make this a hard failure)

>+		LOG(RkISP1, Warning) << "Camera sensor information not available";
>+		sensorInfo = {};
>+		ret = 0;
>+	}
>+
>+	std::map<unsigned int, const ControlInfoMap &> entityControls;
>+	entityControls.emplace(0, data->sensor_->controls());
>+
>+	IPAOperationData ipaConfig;
>+	data->ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig, nullptr);
>+
> 	return 0;
> }
>
>@@ -759,8 +788,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c
> 		return ret;
> 	}
>
>-	std::map<unsigned int, IPAStream> streamConfig;
>-
> 	if (data->mainPath_->isEnabled()) {
> 		ret = mainPath_.start();
> 		if (ret) {
>@@ -770,11 +797,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c
> 			freeBuffers(camera);
> 			return ret;
> 		}
>-
>-		streamConfig[0] = {
>-			.pixelFormat = data->mainPathStream_.configuration().pixelFormat,
>-			.size = data->mainPathStream_.configuration().size,
>-		};
> 	}
>
> 	if (data->selfPath_->isEnabled()) {
>@@ -787,34 +809,11 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c
> 			freeBuffers(camera);
> 			return ret;
> 		}
>-
>-		streamConfig[1] = {
>-			.pixelFormat = data->selfPathStream_.configuration().pixelFormat,
>-			.size = data->selfPathStream_.configuration().size,
>-		};
> 	}
>
> 	isp_->setFrameStartEnabled(true);
>
> 	activeCamera_ = camera;
>-
>-	/* Inform IPA of stream configuration and sensor controls. */
>-	CameraSensorInfo sensorInfo = {};
>-	ret = data->sensor_->sensorInfo(&sensorInfo);
>-	if (ret) {
>-		/* \todo Turn this in an hard failure. */
>-		LOG(RkISP1, Warning) << "Camera sensor information not available";
>-		sensorInfo = {};
>-		ret = 0;
>-	}
>-
>-	std::map<unsigned int, const ControlInfoMap &> entityControls;
>-	entityControls.emplace(0, data->sensor_->controls());
>-
>-	IPAOperationData ipaConfig;
>-	data->ipa_->configure(sensorInfo, streamConfig, entityControls,
>-			      ipaConfig, nullptr);
>-
> 	return ret;
> }
>
>-- 
>2.17.1
>
>_______________________________________________
>libcamera-devel mailing list
>libcamera-devel at lists.libcamera.org
>https://lists.libcamera.org/listinfo/libcamera-devel