<div dir="ltr">Thanks Umang and Han-lin,<div><br></div><div>An interesting fact is that the current patch (v3) actually passes android.hardware.cts.CameraTest#testMultipleCameras. Maybe it only opens the cameras without actually starting them and getting frames?</div><div><br></div><div>Nonetheless, I updated Camera::acquire() and Camera::release() to check with the pipeline handler, and still makes PipelineHandlerIPU3 support only one camera being acquired at a time. Please check.</div><div><br></div><div><span style="color:rgb(80,0,80)">> > int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlList *controls)<br>> > {<br>> > + /*<br>> > + * Enforce that only a single camera can be used at a time to use both<br>> > + * ImgUs on the camera, so that StillCapture stream can adopt another<br>> > + * set of configuration.<br>> > + */<br>> > + if (inUseCamera_ && inUseCamera_ != camera)<br>> > + return -EBUSY;<br><br></span>> I think this is a wrong place to return -EBUSY.<br><br>> After Camera::acquire() success, we fail to start the camera reporting<br>> -EBUSY, is not a good behavior IMO<br></div><div><br></div><div>Right, how about returning -EUSERS as well? With v4, it actually should never happen.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jul 26, 2022 at 11:48 PM Umang Jain <<a href="mailto:umang.jain@ideasonboard.com" target="_blank">umang.jain@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">Hi Hanlin,<br>
<br>
On 7/26/22 17:50, Hanlin Chen wrote:<br>
> On Tue, Jul 26, 2022 at 3:16 PM Umang Jain via libcamera-devel<br>
> <<a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a>> wrote:<br>
>> Hi Harvey,<br>
>><br>
>> Thank you for the patch.<br>
>><br>
>> On 6/29/22 16:00, Harvey Yang via libcamera-devel wrote:<br>
>>> From: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
>>><br>
>>> As we hardly have use cases/applications that need both cameras at the<br>
>>> same time, this patch adds a rule that only one camera can be started<br>
>>> one time. This also allows the following patches that use both imgus to<br>
>><br>
>> Before I go any further, I believe there are CTS tests that require<br>
>> multiple streaming cameras at same point.<br>
>><br>
>> For e.g. I see android.hardware.cts.CameraTest#testMultipleCameras [1]<br>
>><br>
>> Is my understanding correct that streaming multiple cameras at the same<br>
>> is a CTS requirement ? Doesn't this patch violate that?<br>
>><br>
>> I also see a related patch on the list under `[PATCH] libcamera: Allow<br>
>> concurrent use of cameras from same pipeline handler` subject. Would you<br>
>> like to review/discuss or clarify the situation here please?<br>
>><br>
>> [1]<br>
>> <a href="https://android.googlesource.com/platform/cts/+/refs/heads/master/tests/camera/src/android/hardware/camera2/cts/CameraManagerTest.java#337" rel="noreferrer" target="_blank">https://android.googlesource.com/platform/cts/+/refs/heads/master/tests/camera/src/android/hardware/camera2/cts/CameraManagerTest.java#337</a><br>
>><br>
> Hi Umang and Harvey,<br>
><br>
> I think the test accept MAX_CAMERAS_IN_USE error as a pass situation,<br>
> which means HAL rejects the open due to internal resource limit. The<br>
> error is set when CameraDevice::open() returns -EUSER:<br>
> * -EUSERS: The maximal number of camera devices that can be opened<br>
> concurrently were opened already, either by this method or the<br>
> open_legacy method.<br>
> The problem here is that we didn't return -EUSERS on Camera::acquire()<br>
> which is called by CameraDevice::open().<br>
> Instead of checking this in PipelineHandlerIPU3::start(),<br>
> Camera::acquire() could call into the pipeline handler to check the<br>
> resource limit and return the -EUSER error directly, since only the<br>
> pipeline handler knows how to manage its resources.<br>
<br>
<br>
I agree with the premise here, but currently I think pipeline-handlers <br>
don't have a concept of "resource limiting" being implemented; based on <br>
which we decide whether a camera can be acquired or not.<br>
<br>
The patch in discussion below, implements a acquire() per <br>
pipeline-handler so it might be good to extend that to satisfy this case <br>
for IPU3 platform..<br>
<br>
<br>
> See:<br>
> <a href="https://android.googlesource.com/platform/cts/+/refs/heads/master/tests/camera/src/android/hardware/camera2/cts/CameraManagerTest.java#409" rel="noreferrer" target="_blank">https://android.googlesource.com/platform/cts/+/refs/heads/master/tests/camera/src/android/hardware/camera2/cts/CameraManagerTest.java#409</a><br>
> <a href="https://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/camera_common.h#872" rel="noreferrer" target="_blank">https://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/camera_common.h#872</a><br>
><br>
> I think "[PATCH] libcamera: Allow concurrent use of cameras from same<br>
> pipeline handler" is resolving a different problem. It allows opening<br>
> multi cameras if resources are adequate. Our case is to report the<br>
> error accordingly when resources are inadequate.<br>
><br>
>>> process frames from one single camera.<br>
>>><br>
>>> Signed-off-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
>>> ---<br>
>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 16 ++++++++++++++++<br>
>>> 1 file changed, 16 insertions(+)<br>
>>><br>
>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
>>> index fd989e61..c943ee6a 100644<br>
>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
>>> @@ -166,6 +166,8 @@ private:<br>
>>> MediaDevice *cio2MediaDev_;<br>
>>> MediaDevice *imguMediaDev_;<br>
>>><br>
>>> + Camera *inUseCamera_ = nullptr;<br>
>>> +<br>
>>> std::vector<IPABuffer> ipaBuffers_;<br>
>>> };<br>
>>><br>
>>> @@ -765,6 +767,14 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)<br>
>>><br>
>>> int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlList *controls)<br>
>>> {<br>
>>> + /*<br>
>>> + * Enforce that only a single camera can be used at a time to use both<br>
>>> + * ImgUs on the camera, so that StillCapture stream can adopt another<br>
>>> + * set of configuration.<br>
>>> + */<br>
>>> + if (inUseCamera_ && inUseCamera_ != camera)<br>
>>> + return -EBUSY;<br>
>><br>
>> I think this is a wrong place to return -EBUSY.<br>
>><br>
>> After Camera::acquire() success, we fail to start the camera reporting<br>
>> -EBUSY, is not a good behavior IMO<br>
>><br>
>>> +<br>
>>> IPU3CameraData *data = cameraData(camera);<br>
>>> CIO2Device *cio2 = &data->cio2_;<br>
>>> ImgUDevice *imgu = data->imgu_;<br>
>>> @@ -781,6 +791,8 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis<br>
>>> if (ret)<br>
>>> return ret;<br>
>>><br>
>>> + inUseCamera_ = camera;<br>
>>> +<br>
>>> ret = data->ipa_->start();<br>
>>> if (ret)<br>
>>> goto error;<br>
>>> @@ -808,6 +820,8 @@ error:<br>
>>> freeBuffers(camera);<br>
>>> LOG(IPU3, Error) << "Failed to start camera " << camera->id();<br>
>>><br>
>>> + inUseCamera_ = nullptr;<br>
>>> +<br>
>>> return ret;<br>
>>> }<br>
>>><br>
>>> @@ -826,6 +840,8 @@ void PipelineHandlerIPU3::stopDevice(Camera *camera)<br>
>>> LOG(IPU3, Warning) << "Failed to stop camera " << camera->id();<br>
>>><br>
>>> freeBuffers(camera);<br>
>>> +<br>
>>> + inUseCamera_ = nullptr;<br>
>>> }<br>
>>><br>
>>> void IPU3CameraData::cancelPendingRequests()<br>
</blockquote></div>