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

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2020-09-14 21:42:27 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> 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?

> 
> > 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

-- 
Regards,
Niklas Söderlund