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

14052 diff mbox series

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.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/ipa/raspberrypi/raspberrypi.cpp                | 13 +++++++++----
 src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  3 ++-
 2 files changed, 11 insertions(+), 5 deletions(-)

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@raspberrypi.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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())

Hi David,

Thank you for this fix.

On Tue, 5 Oct 2021 at 09:57, David Plowman <david.plowman@raspberrypi.com>
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.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>

Reviewed-by: Naushir Patuck <naush@raspberrypi.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())
> --
> 2.20.1
>
>