[libcamera-devel] [PATCH v2] cam: event_loop: Stop processing calls when event loop is exiting

[Sebastian Fricke]

Hey Niklas & Laurent,

On 20.01.2021 10:12, Niklas Söderlund wrote:
>Hi Laurent,
>
>Thanks for your feedback.
>
>On 2021-01-20 05:56:46 +0200, Laurent Pinchart wrote:
>> Ni Niklas,
>>
>> Thank you for the patch.
>>
>> On Wed, Jan 20, 2021 at 12:33:15AM +0100, Niklas Söderlund wrote:
>> > Terminating the event loop with EventLoop::exit() does not grantee that
>> > it will terminate. If the event loops 'call later' queue can be kept
>> > non-empty by continuously queuing new calls using EventLoop::callLater()
>> > either from a different thread or from callbacks in the loop itself
>> > EventLoop::dispatchCalls() will never complete and the loop will run
>> > forever.
>> >
>> > Solve this by evaluating the exit condition between each callback and
>> > only enter the idle loop if there are no callbacks to process.
>> >
>> > Reported-by: Sebastian Fricke <sebastian.fricke at posteo.net>
>> > Fixes: f49e93338b6309a6 ("cam: event_loop: Add deferred calls support")
>> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>>
>> This seems to be even more of a hack than the previous version :-( Can
>> we fix this properly please ?
>
>I would like noting more then to fix this properly.
>
>The heart of the problem is that some of our pipelines require more then
>N requests to be queued for it to complete N. I agree it's bad that our
>pipelines behaves differently and something should be done to align
>them.
>
>It feels natural that such work would be paired with a compliance tool
>so situations like this can be detected early and avoided. I have been
>tempted many times to start a compliance tool but have postponed it. My

I am currently looking for a follow up project to start within the next 2-3
weeks. If you would like to we could talk about your idea and design, so
that I am able to start working on it, as soon as my current projects
are finished.

>primary reason for doing so is that we never really defined the number
>of requests queued vs. pipeline depth and pipeline stalls and what a
>correct behavior in such situations is. I was pleased with the camera
>model work had was hoping it would lead to a more strict definition wich
>such tools could be built upon.
>
>How ever solving the core problem is a rather large task to sort out.

If I understood your discussion correctly, then the sub-tasks of this
one 'bigger' task are:
- Make sure that each pipeline requies exactly the expected amount of
   requests in order process the given amount of frames, by implementing
   a complience tool to check the pipelines.
- Switch the behavior of the `cam` command to count the number of queued
   requests to the camera instead of the number of completed requests.

Which other tasks would you add to that list?

>How can we handle the issue in the short time frame? No matter how I
>dice it I see a hack in cam is needed ;-(
>
>- The best option I see is to use the 'process call in batches' solution
>  as your suggestion in v1 and then either remove support for
>  --capture=N or accept that on "broken" pipelines N+pipeline depth
>  frames will be processed, but at least cam won't deadlock.
>
>- Another other option (that I really don't like) is to revert the
>  deferred calls support.
>
>- We can also fix this properly in cam by only queuing N requests and
>  expecting N requests to complete. This will break --capture=N on some
>  pipelines, but perhaps that is the pressure we need to push the fix
>  upwards.

I think I like the idea of starting out with the right behavior in the
first place. Are there any cases known to you guys, where the `cam`
application is used for anything else than testing the libcamera project?
Like is there anything that could actually "break"?

>
>>
>> > ---
>> > * Changes since v1
>> > - Was [PATCH 0/2] cam: Fix races in event loop and long request
>> >   processing times
>> > - Rework dispatchCalls() instead of callLater() for a simpler fix.
>> > ---
>> >  src/cam/event_loop.cpp | 22 +++++++++++++---------
>> >  src/cam/event_loop.h   |  2 +-
>> >  2 files changed, 14 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
>> > index 94c5d1d362455f33..18786b4500be9b6f 100644
>> > --- a/src/cam/event_loop.cpp
>> > +++ b/src/cam/event_loop.cpp
>> > @@ -41,7 +41,9 @@ int EventLoop::exec()
>> >  	exit_.store(false, std::memory_order_release);
>> >
>> >  	while (!exit_.load(std::memory_order_acquire)) {
>> > -		dispatchCalls();
>> > +		if (dispatchCall())
>> > +			continue;
>> > +
>> >  		event_base_loop(event_, EVLOOP_NO_EXIT_ON_EMPTY);
>> >  	}
>> >
>> > @@ -70,17 +72,19 @@ void EventLoop::callLater(const std::function<void()> &func)
>> >  	interrupt();
>> >  }
>> >
>> > -void EventLoop::dispatchCalls()
>> > +bool EventLoop::dispatchCall()
>> >  {
>> >  	std::unique_lock<std::mutex> locker(lock_);
>> >
>> > -	for (auto iter = calls_.begin(); iter != calls_.end(); ) {
>> > -		std::function<void()> call = std::move(*iter);
>> > +	if (calls_.empty())
>> > +		return false;
>> >
>> > -		iter = calls_.erase(iter);
>> > +	auto iter = calls_.begin();
>> > +	std::function<void()> call = std::move(*iter);
>> > +	calls_.erase(iter);
>> >
>> > -		locker.unlock();
>> > -		call();
>> > -		locker.lock();
>> > -	}
>> > +	locker.unlock();
>> > +	call();
>> > +
>> > +	return true;
>> >  }
>> > diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h
>> > index 408073c50594d09d..b84e1c9e432086e3 100644
>> > --- a/src/cam/event_loop.h
>> > +++ b/src/cam/event_loop.h
>> > @@ -38,7 +38,7 @@ private:
>> >  	std::mutex lock_;
>> >
>> >  	void interrupt();
>> > -	void dispatchCalls();
>> > +	bool dispatchCall();
>> >  };
>> >
>> >  #endif /* __CAM_EVENT_LOOP_H__ */
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
>
>-- 
>Regards,
>Niklas Söderlund

Greetings,
Sebastian Fricke
>_______________________________________________
>libcamera-devel mailing list
>libcamera-devel at lists.libcamera.org
>https://lists.libcamera.org/listinfo/libcamera-devel