[libcamera-devel] [PATCH v2 1/1] pipeline: raspberrypi: Create empty control lists correctly

[Laurent Pinchart]

Hi David,

Thank you for the patch.

On Tue, Oct 05, 2021 at 09:57:00AM +0100, David Plowman wrote:
> When the pipeline handler start() method is supplied with a NULL list
> of controls, we send an empty control list to the IPA. When the IPA is
> running in isolated mode the control list goes through the data
> serializer, for which it must be marked correctly as a list of
> "controls::controls", otherwise the IPA process will abort.
> 
> The IPA has a similar problem returning a control list in its
> configure() method. We must be careful to initialise it properly even
> when empty.

Looks good to me. I'll wait another day before pushing this to let Paul
chime in if he wants to.

> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/ipa/raspberrypi/raspberrypi.cpp                | 13 +++++++++----
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  3 ++-
>  2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 047123ab..fed82e22 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -389,21 +389,26 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
>  	/* Pass the camera mode to the CamHelper to setup algorithms. */
>  	helper_->SetCameraMode(mode_);
>  
> +	/*
> +	 * Initialise this ControlList correctly, even if empty, in case the IPA is
> +	 * running is isolation mode (passing the ControlList through the IPC layer).
> +	 */
> +	ControlList ctrls(sensorCtrls_);
> +
>  	if (firstStart_) {
>  		/* Supply initial values for frame durations. */
>  		applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);
>  
>  		/* Supply initial values for gain and exposure. */
> -		ControlList ctrls(sensorCtrls_);
>  		AgcStatus agcStatus;
>  		agcStatus.shutter_time = defaultExposureTime;
>  		agcStatus.analogue_gain = defaultAnalogueGain;
>  		applyAGC(&agcStatus, ctrls);
> -
> -		ASSERT(controls);
> -		*controls = std::move(ctrls);
>  	}
>  
> +	ASSERT(controls);
> +	*controls = std::move(ctrls);
> +
>  	return 0;
>  }
>  
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 0bdfa727..1634ca98 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -825,7 +825,8 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
>  
>  	/* Start the IPA. */
>  	ipa::RPi::StartConfig startConfig;
> -	data->ipa_->start(controls ? *controls : ControlList{}, &startConfig);
> +	data->ipa_->start(controls ? *controls : ControlList{ controls::controls },
> +			  &startConfig);
>  
>  	/* Apply any gain/exposure settings that the IPA may have passed back. */
>  	if (!startConfig.controls.empty())

-- 
Regards,

Laurent Pinchart