[libcamera-devel] [PATCH v2 2/8] utils: ipc: proxy: Process pending messages

[Laurent Pinchart]

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 at 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 at ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham at 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 -%}
> >  

-- 
Regards,

Laurent Pinchart