<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Aug 30, 2024 at 12:07 AM 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 Thu, Aug 29, 2024 at 11:34:13PM +0200, Cheng-Hao Yang wrote:<br>
> On Thu, Aug 29, 2024 at 10:59 PM Laurent Pinchart wrote:<br>
> > 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<br>
> > InThread mode?<br>
> <br>
> Just realized that ipu3's proxy is created in `PipelineHandler::match`,<br>
> which is called on the pipeline handler thread, and it's never destructed.<br>
<br>
Yes that's the reason. Sorry, I forgot to reply to your question.<br></blockquote><div><br></div><div>No worries. Thanks for the confirmation.</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">
<br>
> > > 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>
> <br>
> Right. I think the easiest way is to construct the IPA proxy during<br>
> `PipelineHandler::match`, where a camera is registered, and choose to<br>
> destruct it right after using it to list controls.<br>
> <br>
> It can also be kept there, if proprietary libraries don't consume lots of<br>
> memory just to list controls before starting to process statistics. The<br>
> `acquireDevice()` would need to check if an IPA proxy already exists<br>
> or not though.<br>
<br>
Those could be options, yes. Neither sound optimal, but I'm not sure if<br>
there's a better solution. Let's sleep over this. I'd also be happy to<br>
forget about the problem, but I'm sure you'll remind me with patches :-)<br></blockquote><div><br></div><div>Actually, I might not :)</div><div>In mtkisp7, the proprietary libraries don't provide the control list, and all the</div><div>controls are hard-coded in the match() function. I don't think Intel's proprietary</div><div>libraries provide the info either.<br>Let's see though haha...<br><br>BR,<br>Harvey</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">
<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></div>