[libcamera-devel] [PATCH] libcamera: pipeline: rkisp1: Guard against IPA posting actions when we have no camera

[Laurent Pinchart]

Hi Niklas,

On Tue, Sep 15, 2020 at 12:00:57AM +0200, Niklas Söderlund wrote:
> On 2020-09-14 21:42:27 +0300, Laurent Pinchart wrote:
> > On Mon, Sep 14, 2020 at 04:02:17PM +0200, Niklas Söderlund wrote:
> > > The IPA is running asynchronously from the pipeline and may be in the
> > > process of completing some action while the pipeline is stopping the
> > > camera. Prevent processing actions after the camera is stopped by
> > > checking that the pipeline is running with an active camera or not.
> > 
> > Have you seen this happening ? The right way to handle such issues
> > should be to stop the IPA synchronously when stopping the camera, and
> > ensuring that all pending messages are delivered.
> 
> I noticed it when playing around with some local hacks but not with 
> something real. I thought it worth a guard as right now an out-of-tree 
> IPA could very well misbehave and queue actions before/after 
> start()/stop(). But maybe this is better addressed in the new IPC layer 
> to make it behave the same for all pipelines/IPAs?

The IPA stop() function is called asynchronously across threads, and the
IPA proxy waits for the IPA to reply to make the call synchronous from a
PH point of view. All messages sent from the IPA to the PH are thus
processed by the PH thread before the stop() call returns. The only
possibility of error (if I'm not mistaken) would be if the IPA queues a
frame action *after* returning from stop(). I think that would be a bug
in the IPA. We could add assertions in the proxy to check for that, I
think that would be better than adding checks in all PHs.

> > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > ---
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index a6fc3b8e36f3b00a..73d1e9c4ef21fd45 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -413,6 +413,11 @@ int RkISP1CameraData::loadIPA()
> > >  void RkISP1CameraData::queueFrameAction(unsigned int frame,
> > >  					const IPAOperationData &action)
> > >  {
> > > +	/* Guard again IPA queuing actions when we have no camera. */
> > > +	PipelineHandlerRkISP1 *pipe = static_cast<PipelineHandlerRkISP1 *>(pipe_);
> > > +	if (!pipe->activeCamera_)
> > > +		return;
> > > +
> > >  	switch (action.operation) {
> > >  	case RKISP1_IPA_ACTION_V4L2_SET: {
> > >  		const ControlList &controls = action.controls[0];

-- 
Regards,

Laurent Pinchart