[libcamera-devel] [PATCH v3 2/9] ipu3: Allow only one camera being started

[Umang Jain]

Hi Hanlin,

On 7/26/22 17:50, Hanlin Chen wrote:
> On Tue, Jul 26, 2022 at 3:16 PM Umang Jain via libcamera-devel
> <libcamera-devel at lists.libcamera.org> wrote:
>> Hi Harvey,
>>
>> Thank you for the patch.
>>
>> On 6/29/22 16:00, Harvey Yang via libcamera-devel wrote:
>>> From: Harvey Yang <chenghaoyang at chromium.org>
>>>
>>> As we hardly have use cases/applications that need both cameras at the
>>> same time, this patch adds a rule that only one camera can be started
>>> one time. This also allows the following patches that use both imgus to
>>
>> Before I go any further, I believe there are CTS tests that require
>> multiple streaming cameras at same point.
>>
>> For e.g. I see android.hardware.cts.CameraTest#testMultipleCameras [1]
>>
>> Is my understanding correct that streaming multiple cameras at the same
>> is a CTS requirement ? Doesn't this patch violate that?
>>
>> I also see a related patch on the list under `[PATCH] libcamera: Allow
>> concurrent use of cameras from same pipeline handler` subject. Would you
>> like to review/discuss or clarify the situation here please?
>>
>> [1]
>> https://android.googlesource.com/platform/cts/+/refs/heads/master/tests/camera/src/android/hardware/camera2/cts/CameraManagerTest.java#337
>>
> Hi Umang and Harvey,
>
> I think the test accept MAX_CAMERAS_IN_USE error as a pass situation,
> which means HAL rejects the open due to internal resource limit. The
> error is set when CameraDevice::open() returns -EUSER:
> * -EUSERS: The maximal number of camera devices that can be opened
> concurrently were opened already, either by this method or the
> open_legacy method.
> The problem here is that we didn't return -EUSERS on Camera::acquire()
> which is called by CameraDevice::open().
> Instead of checking this in PipelineHandlerIPU3::start(),
> Camera::acquire() could call into the pipeline handler to check the
> resource limit and return the -EUSER error directly, since only the
> pipeline handler knows how to manage its resources.


I agree with the premise here, but currently I think pipeline-handlers 
don't have a concept of "resource limiting" being implemented; based on 
which we decide whether a camera can be acquired or not.

The patch in discussion below, implements a acquire() per 
pipeline-handler so it might be good to extend that to satisfy this case 
for IPU3 platform..


> See:
> https://android.googlesource.com/platform/cts/+/refs/heads/master/tests/camera/src/android/hardware/camera2/cts/CameraManagerTest.java#409
> https://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/camera_common.h#872
>
> I think "[PATCH] libcamera: Allow concurrent use of cameras from same
> pipeline handler" is resolving a different problem. It allows opening
> multi cameras if resources are adequate. Our case is to report the
> error accordingly when resources are inadequate.
>
>>> process frames from one single camera.
>>>
>>> Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
>>> ---
>>>    src/libcamera/pipeline/ipu3/ipu3.cpp | 16 ++++++++++++++++
>>>    1 file changed, 16 insertions(+)
>>>
>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> index fd989e61..c943ee6a 100644
>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> @@ -166,6 +166,8 @@ private:
>>>        MediaDevice *cio2MediaDev_;
>>>        MediaDevice *imguMediaDev_;
>>>
>>> +     Camera *inUseCamera_ = nullptr;
>>> +
>>>        std::vector<IPABuffer> ipaBuffers_;
>>>    };
>>>
>>> @@ -765,6 +767,14 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)
>>>
>>>    int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlList *controls)
>>>    {
>>> +     /*
>>> +      * Enforce that only a single camera can be used at a time to use both
>>> +      * ImgUs on the camera, so that StillCapture stream can adopt another
>>> +      * set of configuration.
>>> +      */
>>> +     if (inUseCamera_ && inUseCamera_ != camera)
>>> +             return -EBUSY;
>>
>> I think this is a wrong place to return -EBUSY.
>>
>> After Camera::acquire() success, we fail to start the camera reporting
>> -EBUSY, is not a good behavior IMO
>>
>>> +
>>>        IPU3CameraData *data = cameraData(camera);
>>>        CIO2Device *cio2 = &data->cio2_;
>>>        ImgUDevice *imgu = data->imgu_;
>>> @@ -781,6 +791,8 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis
>>>        if (ret)
>>>                return ret;
>>>
>>> +     inUseCamera_ = camera;
>>> +
>>>        ret = data->ipa_->start();
>>>        if (ret)
>>>                goto error;
>>> @@ -808,6 +820,8 @@ error:
>>>        freeBuffers(camera);
>>>        LOG(IPU3, Error) << "Failed to start camera " << camera->id();
>>>
>>> +     inUseCamera_ = nullptr;
>>> +
>>>        return ret;
>>>    }
>>>
>>> @@ -826,6 +840,8 @@ void PipelineHandlerIPU3::stopDevice(Camera *camera)
>>>                LOG(IPU3, Warning) << "Failed to stop camera " << camera->id();
>>>
>>>        freeBuffers(camera);
>>> +
>>> +     inUseCamera_ = nullptr;
>>>    }
>>>
>>>    void IPU3CameraData::cancelPendingRequests()