[PATCH 2/5] pipeline_handler: Call releaseDevice() before unlocking media devices

[Cheng-Hao Yang]

Hi Hans,

Thanks for the patch. I've tested it on mtkisp7, and it works fine.

Reviewed-by: Harvey Yang <chenghaoyang at chromium.org>

One question that might not be related though:
`PipelineHandler::release` is defined to be thread safe, while
there's no such description for `PipelineHandler::releaseDevice`.
In mtkisp7, we have a CL [1] to make sure
`PipelineHandler::releaseDevice` is always blockingly executed
on  PipelineHandler's thread.

Although it makes the normal cases (that it's returning true
directly) run longer, as it waits for another thread's execution,
it prevents PipelineHandler's mis-implementation.
I'd suggest either we do the same in the upstream, or we add
the description of being thread-safe for
`PipelineHandler::releaseDevice` to remind the developers of
pipeline handlers.

WDYT?

BR,
Harvey

[1]:
https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5528925

On Tue, Aug 20, 2024 at 9:50 PM Hans de Goede <hdegoede at redhat.com> wrote:

> It is better / more logical to call releaseDevice() before unlocking
> the devices. ATM the only pipeline handler implementing releaseDevice()
> is the rpi pipeline handler which releases buffers from its releaseDevice()
> implementation.
>
> Releasing buffers before unlocking the media devices is ok to do
> and arguably it is better to release the buffers before unlocking.
>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
>  src/libcamera/pipeline_handler.cpp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/libcamera/pipeline_handler.cpp
> b/src/libcamera/pipeline_handler.cpp
> index a20cd27d..1fc22d6a 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -205,11 +205,11 @@ void PipelineHandler::release(Camera *camera)
>
>         ASSERT(useCount_);
>
> +       releaseDevice(camera);
> +
>         if (useCount_ == 1)
>                 unlockMediaDevices();
>
> -       releaseDevice(camera);
> -
>         --useCount_;
>  }
>
> --
> 2.46.0
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20240821/8deb8918/attachment.htm>