[libcamera-devel] [PATCH] libcamera: pipeline: ipu3: Stop IPA before stopping devices

[Laurent Pinchart]

Hi Kieran,

On Mon, Mar 08, 2021 at 04:40:56PM +0000, Kieran Bingham wrote:
> On 08/03/2021 16:37, Niklas Söderlund wrote:
> > On 2021-03-08 16:29:15 +0000, Kieran Bingham wrote:
> >> The IPA should be stopped before the hardware devices to ensure that
> >> all asynchronous actions have completed within the IPA before resources
> >> are removed and released.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >>  src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> index 2b4d31500533..498f85634a83 100644
> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> @@ -764,13 +764,13 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> >>  	IPU3CameraData *data = cameraData(camera);
> >>  	int ret = 0;
> >>  
> >> +	data->ipa_->stop();
> >> +
> >>  	ret |= data->imgu_->stop();
> >>  	ret |= data->cio2_.stop();
> >>  	if (ret)
> >>  		LOG(IPU3, Warning) << "Failed to stop camera " << camera->id();
> >>  
> >> -	data->ipa_->stop();
> >> -
> > 
> > IIRC correctly this order was picked because before the cio2 is stopped 
> > buffers may still be dequeued and therefor handed to the IPA for 
> > processing. This was however done before the new IPC change and with a 
> > mock IPA. How does this work with the new IPC/IPA if the pipeline 
> > handler tries to post buffers to be processed before the CIO2 is 
> > stopped?
> 
> The IPA before stopped sends events that cause thigns to be queued to
> the v4l2 devices.
> 
> This hits the fact that the v42l buffer cache is null because the
> buffers have been released.
> 
> So we must stop the IPA before the devices..
> 
> But then we must also make sure the IPA is not processing frames and
> events from buffer completions after it has stopped....

I don't think this can happen, as V4L2 events are processed in the event
loop, and we don't return to the event loop between data->ipa_->stop()
and the imgu and cio2 stop calls. The IPA stop() call is different, as
it calls IPCPipeUnixSocket::call(), which has a manual call to
Thread::current()->eventDispatcher()->processEvents() to wait for the
IPA response to the stop message.

There's a race condition there, as other events could also be processed.
We need to extend the EventDispatcher::processEvents() API to specify
event types (or something similar).

> >>  	freeBuffers(camera);
> > 
> > Should not all resources shared between pipeline and IPA be available 
> > until the buffers are freed here?
> 
> We're battling a use-after-free I think at the minute so indeed, this
> could be relevant.
> 
> >>  }
> >>  

-- 
Regards,

Laurent Pinchart