[PATCH] libcamera: rkisp1: Only connect delayed controls at start/stop

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Fri, Feb 07, 2025 at 01:37:43PM +0000, Kieran Bingham wrote:
> The RKISP1 path may potentially have multiple cameras connected through
> complex pipelines such as video multiplexors or multiple FPGA paths.
> 
> The RKISP1 pipeline handler notifies DelayedControls that a new frame is
> commencing by using the frameStart event on the ISP and using that to
> signal to DelayedControls that it is time to process the controls for
> the next frame.
> 
> When more than one camera is connected to an ISP it is important not to
> signal events to an inactive Camera.
> 
> Move the frameStart signal connection from CreateCamera() to start() and
> introduce a corresponding disconnect at stop().
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> 
> When developing on an i.MX8MP, with multiple cameras connected to a
> single ISP this caused a horrible bug which I eventually found using
> UBSAN/ASAN to spot that actaully there was a use-after-free in calling
> delayedControls on a camera object that wasn't active.

I've been pondering about requiring the receiver of a signal to inherit
from the Object class. That would solve this kind of issues.

> One other alternative here, could be to move the connection of the
> signal into the setFrameStartEnabled() call, if we pass in the
> CameraData to be explicit on where we are connecting and disconnect. But
> maybe that's overkill too.

Conceptually, the isp_->frameStart signal is emitted by a resource
shared by multiple cameras. It would make sense to model this with a
slot in the PipelineHandlerRkISP1 class instead of the RkISP1CameraData
class, and relay it to the delayed controls class through activeCamera_.
Would that be a better model ?

>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 1ac8d8ae7ed9..b1b0bde7fd00 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -1071,6 +1071,11 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL
>  	utils::ScopeExitActions actions;
>  	int ret;
>  
> +	isp_->frameStart.connect(data->delayedCtrls_.get(),
> +				 &DelayedControls::applyControls);
> +
> +	actions += [&]() { isp_->frameStart.disconnect(data->delayedCtrls_.get()); };
> +
>  	/* Allocate buffers for internal pipeline usage. */
>  	ret = allocateBuffers(camera);
>  	if (ret)
> @@ -1142,6 +1147,8 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)
>  
>  	isp_->setFrameStartEnabled(false);
>  
> +	isp_->frameStart.disconnect(data->delayedCtrls_.get());
> +
>  	data->ipa_->stop();
>  
>  	if (hasSelfPath_)
> @@ -1330,8 +1337,6 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>  	data->delayedCtrls_ =
>  		std::make_unique<DelayedControls>(data->sensor_->device(),
>  						  params);
> -	isp_->frameStart.connect(data->delayedCtrls_.get(),
> -				 &DelayedControls::applyControls);
>  
>  	ret = data->loadIPA(media_->hwRevision());
>  	if (ret)

-- 
Regards,

Laurent Pinchart