<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Aug 25, 2024 at 2:27 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 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></blockquote><div><br></div><div>Yes, you're right. Thanks for the clarification.</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">
> 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></blockquote><div><br></div><div>The main issue that CrOS wants to solve is: We use acquire() & release()</div><div>to handle the IPA (proxy & the sandboxed process)'s lifetime [1][2]. Within the </div><div>current libcamera API, the IPA should be active when configure() is called,</div><div>and therefore it doesn't make sense to terminate the IPA (and release some</div><div>DMA buffers) when the camera is stopped.</div><div>acquire() and release() are the best places to construct & destruct the IPA</div><div>proxy & sandboxed process.</div><div>The reason that we want to destruct the IPA proxy at some point is that it's</div><div>the easiest way to clean up proprietary libraries' memory usage, which is</div><div>different from how ipu3 works now. ipu3 creates the proxy in match() and</div><div>never destructs it.</div><div><br></div><div>[1]: <a href="https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=1290">https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=1290</a></div><div>[2]: <a href="https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=1298">https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=1298</a></div><div><br></div><div>However, IIUC, an IPA proxy needs to be constructed, used, and destructed</div><div>on the same thread. (Please correct me if I'm wrong.)</div><div>During the development, we spent some time debugging this issue.</div><div>I think ipu3's proxy doesn't run into such an issue because it uses InThread</div><div>mode?</div><div>Therefore, I think it'd be better to at least leave comments to remind developers</div><div>that releaseDevice() (and the potential acquireDevice()) might be called from</div><div>any thread.</div><div><br></div><div>I've just tried to call releaseDevice() directly in release() without switching the</div><div>thread, and got a FATAL error:<br><font face="arial, sans-serif" color="#000000" style="background-color:rgb(255,255,255)">```<br>
</font><p class="gmail-p1" style="margin:0px;font-variant-numeric:normal;font-variant-east-asian:normal;font-variant-alternates:normal;font-size-adjust:none;font-kerning:auto;font-feature-settings:normal;font-stretch:normal;line-height:normal"><font face="arial, sans-serif" style="background-color:rgb(255,255,255)" color="#000000"><span class="gmail-s1" style="font-variant-ligatures:no-common-ligatures">FATAL </span><span class="gmail-s2" style="font-variant-ligatures:no-common-ligatures">default </span><span class="gmail-s3" style="font-variant-ligatures:no-common-ligatures">event_dispatcher_poll.cpp:285 </span><span class="gmail-s4" style="font-variant-ligatures:no-common-ligatures">assertion "iter != notifiers_.end()" failed in processNotifiers()<br>```</span></font></p></div><div><br></div><div>Please also check if constructing the IPA proxy in acquireDevice() makes </div><div>sense.</div><div><br></div><div>Thanks!</div><div><br></div><div>BR,</div><div>Harvey</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">
> 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>