[libcamera-devel] [PATCH 0/3] IPU3 Stability

[Laurent Pinchart]

Hi Kieran,

On Wed, Mar 03, 2021 at 05:04:23PM +0000, Kieran Bingham wrote:
> While working on the IPU3 IPA, some crashes have been occuring on
> shutdown races.
> 
> Patches 2/3 and 3/3 at least presently work around those issues, and
> while they may be suitable for integration now - both suggest that more
> work is needed to ensure that the IPU3 pipeline handler is stable and
> implemented correctly.
> 
> Patch 2/3 highlights that we may need to take more concern over the
> lifetime management of a Request and the associated FrameInfo that is
> supported with that.

I think 2/3 is suitable for integration, even if API improvements are
possible. This isn't a completely unexpected situation, now that we have
an implementation we find the related issues, that's a way to improve
APIs.

> Patch 3/3 shows that the IPU3 Pipeline handler is queueing buffers after
> it has released the buffers on the video nodes, and needs further
> consideration as well (though I beleive that patch is still worthy of
> integration on it's own).

I have more doubts about this one, as there's really something that
should be fixed on the pipeline handler side. I was considering
proposing the Fatal log level, but I suppose it will not make a big
difference compared to a segfault :-) Or maybe it would, the log message
can then clearly state that the problem is in the caller.

> The issue leading to what would be a crash without patch 3/3 is when
> shutting down, the IPA running in parallel can emit an event from:
>  void IPU3CameraData::queueFrameAction()
> as 
> 
>   case ipa::ipu3::ActionParamFilled:
> 
> which then goes on to queue a buffer to the three relevant V4L2 devices:
> 
> 	imgu_->param_->queueBuffer(info->paramBuffer);
> 	imgu_->stat_->queueBuffer(info->statBuffer);
> 	imgu_->input_->queueBuffer(info->rawBuffer);
> 
> 
> This is occuring after those devices have released their buffers, and
> thus the v4l2 buffer cache (cache_) is deleted and set to nullptr,
> resulting in a segmentation fault within the V4L2VideoDevice otherwise.

Looking at PipelineHandlerIPU3::stop(), we have

	data->ipa_->stop();

	freeBuffers(camera);

The IPA stop() call should be synchronous, which means that any pending
asynchronous call from the IPA module to the pipeline handler should be
processed before stop() returns.

On the IPA side, the stop() function should stop any internal thread,
which will make sure that the API won't emit any event on its own after
stop() returns. The stop() implementation is currently empty, but as
we're not creating any custom thread in the IPU3 IPA at this time, it
shouldn't be a problem.

It could also be that the pipeline handler sends more frame events to
the IPA module after stop(), but given the cross-thread nature of the
calls, I wouldn't expect that to go through if the IPA thread created by
the proxy is stopped. Still, it may be worth double-checking this.

Tracing the IPA calls could be useful here. Please let me know if I can
help.

> Kieran Bingham (3):
>   libcamera: pipeline: ipu3: Fix spelling error
>   libcamera: pipeline: ipu3: Ensure that IPU3Frames::info is not used
>     after delete
>   libcamera: v4l2_videodevice: Prevent queueing buffers without a cache
> 
>  src/libcamera/pipeline/ipu3/frames.cpp |  2 +-
>  src/libcamera/pipeline/ipu3/ipu3.cpp   | 20 ++++++++++++++++++--
>  src/libcamera/v4l2_videodevice.cpp     | 10 ++++++++++
>  3 files changed, 29 insertions(+), 3 deletions(-)
> 

-- 
Regards,

Laurent Pinchart