<div dir="ltr">Hi Hans,<div><br></div><div>Thanks for the patch. I've tested it on mtkisp7, and it works fine.</div><div><br></div><div>Reviewed-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br></div><div><br></div><div>One question that might not be related though:</div><div>`PipelineHandler::release` is defined to be thread safe, while</div><div>there's no such description for `PipelineHandler::releaseDevice`.</div><div>In mtkisp7, we have a CL [1] to make sure </div><div>`PipelineHandler::releaseDevice` is always blockingly executed</div><div>on  PipelineHandler's thread.</div><div><br></div><div>Although it makes the normal cases (that it's returning true</div><div>directly) run longer, as it waits for another thread's execution,</div><div>it prevents PipelineHandler's mis-implementation.</div><div>I'd suggest either we do the same in the upstream, or we add</div><div>the description of being thread-safe for</div><div>`PipelineHandler::releaseDevice` to remind the developers of</div><div>pipeline handlers.</div><div><br></div><div>WDYT?</div><div><br></div><div>BR,</div><div>Harvey</div><div><br></div><div>[1]: <a href="https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5528925">https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5528925</a></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Aug 20, 2024 at 9:50 PM Hans de Goede <<a href="mailto:hdegoede@redhat.com">hdegoede@redhat.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">It is better / more logical to call releaseDevice() before unlocking<br>
the devices. ATM the only pipeline handler implementing releaseDevice()<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>
 src/libcamera/pipeline_handler.cpp | 4 ++--<br>
 1 file changed, 2 insertions(+), 2 deletions(-)<br>
<br>
diff --git a/src/libcamera/pipeline_handler.cpp 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>
2.46.0<br>
<br>
</blockquote></div>