<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Aug 29, 2024 at 10:59 PM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Sun, Aug 25, 2024 at 01:21:21PM +0200, Cheng-Hao Yang wrote:<br>
> On Sun, Aug 25, 2024 at 2:27 AM Laurent Pinchart wrote:<br>
> > On Wed, Aug 21, 2024 at 06:34:17PM +0200, Cheng-Hao Yang wrote:<br>
> > > Hi Hans,<br>
> > ><br>
> > > Thanks for the patch. I've tested it on mtkisp7, and it works fine.<br>
> > ><br>
> > > Reviewed-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> > ><br>
> > > One question that might not be related though:<br>
> > > `PipelineHandler::release` is defined to be thread safe, while<br>
> > > there's no such description for `PipelineHandler::releaseDevice`.<br>
> > ><br>
> > > In mtkisp7, we have a CL [1] to make sure<br>
> > > `PipelineHandler::releaseDevice` is always blockingly executed<br>
> > > on PipelineHandler's thread.<br>
> > ><br>
> > > Although it makes the normal cases (that it's returning true<br>
> > > directly) run longer, as it waits for another thread's execution,<br>
> > > it prevents PipelineHandler's mis-implementation.<br>
> > > I'd suggest either we do the same in the upstream, or we add<br>
> > > the description of being thread-safe for<br>
> > > `PipelineHandler::releaseDevice` to remind the developers of<br>
> > > pipeline handlers.<br>
> ><br>
> > I don't think releaseDevice() qualifies as thread safe. Here's how<br>
> > libcamera documents thread safety:<br>
> ><br>
> > * - A **reentrant** function may be called simultaneously from multiple<br>
> > * threads if and only if each invocation uses a different instance of the<br>
> > * class. This is the default for all member functions not explictly marked<br>
> > * otherwise.<br>
> > *<br>
> > * - \anchor thread-safe A **thread-safe** function may be called<br>
> > * simultaneously from multiple threads on the same instance of a class. A<br>
> > * thread-safe function is thus reentrant. Thread-safe functions may also be<br>
> > * called simultaneously with any other reentrant function of the same class<br>
> > * on the same instance.<br>
> ><br>
> > Pipeline handlers shouldn't have to deal with releaseDevice() being<br>
> > called concurrently on the same PipelineHandler instance for different<br>
> > cameras, which is why the caller serializes the calls.<br>
> ><br>
> > releaseDevice() should never race with other pipeline handler calls for<br>
> > the same camera. However, it could race with other calls for different<br>
> > cameras. We should document this, or change the behaviour.<br>
> <br>
> Yes, you're right. Thanks for the clarification.<br>
> <br>
> > > WDYT?<br>
> > ><br>
> > > BR,<br>
> > > Harvey<br>
> > ><br>
> > > [1]: <a href="https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5528925" rel="noreferrer" target="_blank">https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5528925</a><br>
> ><br>
> > The commit message doesn't really explain why the change is needed.<br>
> > Could you elaborate, maybe providing an example of incorrect behaviour<br>
> > of a pipeline handler implementation with the existing calling<br>
> > convention ?<br>
> <br>
> The main issue that CrOS wants to solve is: We use acquire() & release()<br>
> to handle the IPA (proxy & the sandboxed process)'s lifetime [1][2]. Within the<br>
> current libcamera API, the IPA should be active when configure() is called,<br>
> and therefore it doesn't make sense to terminate the IPA (and release some<br>
> DMA buffers) when the camera is stopped.<br>
> acquire() and release() are the best places to construct & destruct the IPA<br>
> proxy & sandboxed process.<br>
> The reason that we want to destruct the IPA proxy at some point is that it's<br>
> the easiest way to clean up proprietary libraries' memory usage, which is<br>
> different from how ipu3 works now. ipu3 creates the proxy in match() and<br>
> never destructs it.<br>
<br>
I'd argue that proprietary libraries are the problem here, but that's a<br>
longer term problem.<br>
<br>
> [1]: <a href="https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=1290" rel="noreferrer" target="_blank">https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=1290</a><br>
> [2]: <a href="https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=1298" rel="noreferrer" target="_blank">https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=1298</a><br>
> <br>
> However, IIUC, an IPA proxy needs to be constructed, used, and destructed<br>
> on the same thread. (Please correct me if I'm wrong.)<br>
<br>
They are thread-bound, which means they indeed have to be used and<br>
destroyed in the same thread. Construction can happen in another thread<br>
if the objects are then moved to the pipeline handler thread with<br>
moveToThread(), but it's easier to just construct the objects in the<br>
same thread.<br>
<br>
> During the development, we spent some time debugging this issue.<br>
> I think ipu3's proxy doesn't run into such an issue because it uses InThread mode?<br></blockquote><div><br></div><div>Just realized that ipu3's proxy is created in `PipelineHandler::match`,</div><div>which is called on the pipeline handler thread, and it's never destructed.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> Therefore, I think it'd be better to at least leave comments to remind developers<br>
> that releaseDevice() (and the potential acquireDevice()) might be called from<br>
> any thread.<br>
> <br>
> I've just tried to call releaseDevice() directly in release() without switching the<br>
> thread, and got a FATAL error:<br>
> ```<br>
> <br>
> FATAL default event_dispatcher_poll.cpp:285 assertion "iter !=<br>
> notifiers_.end()" failed in processNotifiers()<br>
> ```<br>
> <br>
> Please also check if constructing the IPA proxy in acquireDevice() makes<br>
> sense.<br>
<br>
In the context of the problem you mention above, yes, it does make sense<br>
(until we can stop dealing with memory leaks from closed-source blobs).<br>
However, it introduces a problem. The IPA module is responsible for<br>
advertising the controls it supports, and applications can expect from<br>
the current API to list those controls before acquiring a camera. That's<br>
a conflicting requirement, how do you think we should address it ?<br></blockquote><div><br>Right. I think the easiest way is to construct the IPA proxy during </div><div>`PipelineHandler::match`, where a camera is registered, and choose to </div><div>destruct it right after using it to list controls.</div><div><br></div><div>It can also be kept there, if proprietary libraries don't consume lots of</div><div>memory just to list controls before starting to process statistics. The</div><div>`acquireDevice()` would need to check if an IPA proxy already exists</div><div>or not though.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> > On Tue, Aug 20, 2024 at 9:50 PM Hans de Goede wrote:<br>
> > ><br>
> > > > It is better / more logical to call releaseDevice() before unlocking<br>
> > > > the devices. ATM the only pipeline handler implementing releaseDevice()<br>
> ><br>
> > s/ATM/At the moment/<br>
> ><br>
> > > > is the rpi pipeline handler which releases buffers from its releaseDevice()<br>
> > > > implementation.<br>
> > > ><br>
> > > > Releasing buffers before unlocking the media devices is ok to do<br>
> > > > and arguably it is better to release the buffers before unlocking.<br>
> > > ><br>
> > > > Signed-off-by: Hans de Goede <<a href="mailto:hdegoede@redhat.com" target="_blank">hdegoede@redhat.com</a>><br>
> ><br>
> > Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
> ><br>
> > > > ---<br>
> > > > src/libcamera/pipeline_handler.cpp | 4 ++--<br>
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)<br>
> > > ><br>
> > > > diff --git a/src/libcamera/pipeline_handler.cpp<br>
> > > > b/src/libcamera/pipeline_handler.cpp<br>
> > > > index a20cd27d..1fc22d6a 100644<br>
> > > > --- a/src/libcamera/pipeline_handler.cpp<br>
> > > > +++ b/src/libcamera/pipeline_handler.cpp<br>
> > > > @@ -205,11 +205,11 @@ void PipelineHandler::release(Camera *camera)<br>
> > > ><br>
> > > > ASSERT(useCount_);<br>
> > > ><br>
> > > > + releaseDevice(camera);<br>
> > > > +<br>
> > > > if (useCount_ == 1)<br>
> > > > unlockMediaDevices();<br>
> > > ><br>
> > > > - releaseDevice(camera);<br>
> > > > -<br>
> > > > --useCount_;<br>
> > > > }<br>
> > > ><br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br></blockquote><div><br></div><div>BR,<br>Harvey </div></div></div>