[PATCH 00/12] libcamera: Hardening against thread race conditions

[Laurent Pinchart]

On Mon, Jan 22, 2024 at 05:30:55PM +0100, Milan Zamazal wrote:
> Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
> 
> > Even if V4L2VideoDevice isn't addressed directly by this series, and the
> > bug report is related to usage of the V4L2VideoDevice class, I believe
> > the issue in the soft ISP code will be caught by the assertions added to
> > the EventNotifier class. Milan, would you be able to test this, and
> > confirm that libcamera now complains loudly ?
> 
> Nice cleanup, thank you!  Yes, this complains immediately and loudly in the
> given case.
> 
> Now a related question is how to deal with threads and instances of classes that
> don't inherit Object.
> 
> I think V4L2VideoDevice::queueBuffer is a good example.  V4L2VideoDevice is
> thread-bound but doesn't inherit Object.  So the caller, unless it has created
> the given instance itself (and being thread-bound itself or tracking its initial
> thread), has no idea about the thread a V4L2VideoDevice instance is bound too,
> even less how to call it there, and even less without worrying about
> implications of asynchronous calls.

That's kind of the point actually, by design: if a class doesn't inherit
from Object, unless some of its functions are explicitly marked as being
thread-safe, then it's not meant to be called from different threads. 

> This means ensuring most things run in the same thread.  This may be easy in
> some cases and less fine in other ones.  It discourages using multiple threads,
> not sure whether this is a good thing or not.  It's not robust in any case.

Yes, it discourages usage of multiple threads. libcamera instead
encourages usage of event loops without blocking calls. Threads can be
used when there's a good reason to do so. As that's uncommon, most
development doesn't have to worry about race conditions. Only when
adding a thread should the developer know what they're doing.

> A possible alternative would be making less things thread-bound.  I suppose
> there are reasons why this is not happening in libcamera.
> 
> Another alternative would be to use Object (as a direct base class or in
> wrappers) or Thread (together with initial thread tracking) to call the right
> thread in more places.  It would be limited to situations where possibly
> asynchronous invocations don't harm if nothing else.
> 
> The call chain in the software ISP branch, DebayerCpu -> SwIspSimple ->
> SimpleCameraData -> V4L2VideoDevice (thread-bound) -> EventNotifier (inheriting
> Object), must be changed to eventually invoke V4L2VideoDevice::queueBuffer in
> the right thread.  Is the recommended practice to examine thoroughly what's
> running where or are there any simpler options?

-- 
Regards,

Laurent Pinchart