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

[Hans de Goede]

Hi,

On 8/25/24 2:27 AM, Laurent Pinchart wrote:
> On Wed, Aug 21, 2024 at 06:34:17PM +0200, Cheng-Hao Yang wrote:
>> 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.
> 
> I don't think releaseDevice() qualifies as thread safe. Here's how
> libcamera documents thread safety:
> 
>  * - A **reentrant** function may be called simultaneously from multiple
>  *   threads if and only if each invocation uses a different instance of the
>  *   class. This is the default for all member functions not explictly marked
>  *   otherwise.
>  *
>  * - \anchor thread-safe A **thread-safe** function may be called
>  *   simultaneously from multiple threads on the same instance of a class. A
>  *   thread-safe function is thus reentrant. Thread-safe functions may also be
>  *   called simultaneously with any other reentrant function of the same class
>  *   on the same instance.
> 
> Pipeline handlers shouldn't have to deal with releaseDevice() being
> called concurrently on the same PipelineHandler instance for different
> cameras, which is why the caller serializes the calls.
> 
> releaseDevice() should never race with other pipeline handler calls for
> the same camera. However, it could race with other calls for different
> cameras. We should document this, or change the behaviour.

releaseDevice() is only called from: PipelineHandler::release() which starts with:

void PipelineHandler::release(Camera *camera)
{
        MutexLocker locker(lock_);

so even if an app is releasing 2 cameras from the same pipeline-handler
at the same time (from 2 different threads) then the releaseDevice() calls
will be serialized.

Should I document for v2 that releaseDevice() and the new acquireDevice() calls
are serialized by the PipelineHandler base class ?

Regards,

Hans



> 
>> WDYT?
>>
>> BR,
>> Harvey
>>
>> [1]: https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5528925
> 
> The commit message doesn't really explain why the change is needed.
> Could you elaborate, maybe providing an example of incorrect behaviour
> of a pipeline handler implementation with the existing calling
> convention ?
> 
>> On Tue, Aug 20, 2024 at 9:50 PM Hans de Goede wrote:
>>
>>> It is better / more logical to call releaseDevice() before unlocking
>>> the devices. ATM the only pipeline handler implementing releaseDevice()
> 
> s/ATM/At the moment/
> 
>>> 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>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.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_;
>>>  }
>>>
>