| Message ID | 20220629103018.4025635-3-chenghaoyang@google.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Harvey, Thank you for the patch. On 6/29/22 16:00, Harvey Yang via libcamera-devel wrote: > From: Harvey Yang <chenghaoyang@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 > process frames from one single camera. > > Signed-off-by: Harvey Yang <chenghaoyang@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()
On Tue, Jul 26, 2022 at 3:16 PM Umang Jain via libcamera-devel <libcamera-devel@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@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. 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@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()
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@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@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@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()
Thanks Umang and Han-lin, 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? 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. > > 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 Right, how about returning -EUSERS as well? With v4, it actually should never happen. On Tue, Jul 26, 2022 at 11:48 PM Umang Jain <umang.jain@ideasonboard.com> wrote: > 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@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@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@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() >
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; + 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()