| Message ID | 20210312054727.852622-3-kieran.bingham@ideasonboard.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran and Laurent, On 2021-03-12 05:47:21 +0000, Kieran Bingham wrote: > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Events may be queued to the pipeline handler between the pipeline > handler entering the ::stop() function, and before the call to stop the > IPA has completed. > > Handle these events by dispatching all pending messages at the proxy > after the IPA has fully stopped. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > utils/ipc/generators/libcamera_templates/proxy_functions.tmpl | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl > index 13dc8fdcab6e..8addc2fad0a8 100644 > --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl > +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl > @@ -31,6 +31,8 @@ > thread_.exit(); > thread_.wait(); > > + Thread::current()->dispatchMessages(Message::Type::InvokeMessage); Is this not similar to the issue we have in cam? What if the events processed here themself queue events will they be processed? I wonder if the cleanest solution to all this would be to stop accepting messages as the first step and return errors for all calls that would generate an event. After we stopped accepting new events we can process the queue until it's empty and once that is done we can stop the IPA. Maybe it's overkill but at least we would make the race windows smaller and easier to reproduce when found as they would only depend on the content of the queue at stop(). > + > running_ = false; > {%- endmacro -%} > > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran On Fri, Mar 12, 2021 at 05:47:21AM +0000, Kieran Bingham wrote: > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Events may be queued to the pipeline handler between the pipeline > handler entering the ::stop() function, and before the call to stop the > IPA has completed. > > Handle these events by dispatching all pending messages at the proxy > after the IPA has fully stopped. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > utils/ipc/generators/libcamera_templates/proxy_functions.tmpl | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl > index 13dc8fdcab6e..8addc2fad0a8 100644 > --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl > +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl > @@ -31,6 +31,8 @@ > thread_.exit(); > thread_.wait(); > > + Thread::current()->dispatchMessages(Message::Type::InvokeMessage); > + > running_ = false; > {%- endmacro -%} > > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Sat, Mar 13, 2021 at 12:20:33AM +0100, Niklas Söderlund wrote: > On 2021-03-12 05:47:21 +0000, Kieran Bingham wrote: > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Events may be queued to the pipeline handler between the pipeline > > handler entering the ::stop() function, and before the call to stop the > > IPA has completed. > > > > Handle these events by dispatching all pending messages at the proxy > > after the IPA has fully stopped. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > utils/ipc/generators/libcamera_templates/proxy_functions.tmpl | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl > > index 13dc8fdcab6e..8addc2fad0a8 100644 > > --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl > > +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl > > @@ -31,6 +31,8 @@ > > thread_.exit(); > > thread_.wait(); > > > > + Thread::current()->dispatchMessages(Message::Type::InvokeMessage); > > Is this not similar to the issue we have in cam? What if the events > processed here themself queue events will they be processed? It's up to the pipeline handler to decide what to do with these events. For instance, if the pipeline handler stop() function stops the IPA first and then stops video nodes, and the events delivered from the IPA result in buffers being queued to the video nodes, those buffers will be cancelled right away when the video nodes are stopped, so it's harmless (even if it wastes a bit of CPU time). On the other hand, if the pipeline handler sends more events to the IPA, the IPA would never be able to process them as it's stopped by the time messages are dispatched here, and that's an error. The pipeline handler may thus need to implement a state machine to correctly process events it receives, depending on what state it is in. In any case, pending events should be delivered before the IPA stop() call returns, as the alternative is to deliver them later, which creates more race conditions (one case e particularly want to avoid is events generated before stop() being delivered after the camera is re-start()ed, and considered by the pipeline handler as events pertaining to the new capture session). > I wonder if > the cleanest solution to all this would be to stop accepting messages as > the first step and return errors for all calls that would generate an > event. After we stopped accepting new events we can process the queue > until it's empty and once that is done we can stop the IPA. I think that's what I meant above with the state machine, right ? I see it as a pipeline handler decision, depending on whether the event handlers can run unconditionally or not. > Maybe it's overkill but at least we would make the race windows smaller > and easier to reproduce when found as they would only depend on the > content of the queue at stop(). > > > + > > running_ = false; > > {%- endmacro -%} > >
Hi Kieran, Thank you for the (my ;-)) patch. On Fri, Mar 12, 2021 at 05:47:21AM +0000, Kieran Bingham wrote: > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Events may be queued to the pipeline handler between the pipeline > handler entering the ::stop() function, and before the call to stop the > IPA has completed. > > Handle these events by dispatching all pending messages at the proxy > after the IPA has fully stopped. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> This can go in a follow-up patch, but we need to document the fact that pipeline handlers shall expect events to be delivered while they're blocked on stop(). > --- > utils/ipc/generators/libcamera_templates/proxy_functions.tmpl | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl > index 13dc8fdcab6e..8addc2fad0a8 100644 > --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl > +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl > @@ -31,6 +31,8 @@ > thread_.exit(); > thread_.wait(); > > + Thread::current()->dispatchMessages(Message::Type::InvokeMessage); > + > running_ = false; > {%- endmacro -%} >
diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl index 13dc8fdcab6e..8addc2fad0a8 100644 --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl @@ -31,6 +31,8 @@ thread_.exit(); thread_.wait(); + Thread::current()->dispatchMessages(Message::Type::InvokeMessage); + running_ = false; {%- endmacro -%}