| Message ID | 20200914140217.54060-1-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Delegated to: | Niklas Söderlund |
| Headers | show |
| Series |
|
| Related | show |
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. > Signed-off-by: Niklas Söderlund <niklas.soderlund@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];
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@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
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@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];
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];
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. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 +++++ 1 file changed, 5 insertions(+)