| Message ID | 20220512103258.324339-3-chenghaoyang@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Harvey Yang via libcamera-devel (2022-05-12 11:32:52) > 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 > process frames from one single camera. I think 'not having many usecases' for something isn't a good reason to forcibly disable it. If the hardware *can not* do it, then that's acceptable. Is there any use case where it is possible to capture from two cameras at the same time? I think it may simply not be possible on the IPU3 - but if that's the case, then that should be the documented rationale behind this patch. If we could for instance capture from both the IR sensor (which may not need the ISP?), and the image sensor at the same time on a front facing camera device like on the SGo2 - then I don't think we should be preventing that here. > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index fd989e61..111ba053 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,10 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera) > > int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlList *controls) > { > + // Deny second camera being started. libcamera code style here for a single line comment should be /* Deny second camera being started. */ But what about the third? If this is /not/ possible - then I think it should be more like: /* * Enforce that only a single camera can be used at a time due * to the limitations of the IPU3 IMGU which can only .. * <detailed of limitation here>. */ > + if (inUseCamera_ && inUseCamera_ != camera) > + return -1; Errnos rather than -1 ... Perhaps -EBUSY ... > + > IPU3CameraData *data = cameraData(camera); > CIO2Device *cio2 = &data->cio2_; > ImgUDevice *imgu = data->imgu_; > @@ -781,6 +787,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 +816,8 @@ error: > freeBuffers(camera); > LOG(IPU3, Error) << "Failed to start camera " << camera->id(); > > + inUseCamera_ = nullptr; > + > return ret; > } > > @@ -826,6 +836,8 @@ void PipelineHandlerIPU3::stopDevice(Camera *camera) > LOG(IPU3, Warning) << "Failed to stop camera " << camera->id(); > > freeBuffers(camera); > + > + inUseCamera_ = nullptr; > } > > void IPU3CameraData::cancelPendingRequests() > -- > 2.36.0.512.ge40c2bad7a-goog >
> I think 'not having many usecases' for something isn't a good reason to > forcibly disable it. If the hardware *can not* do it, then that's > acceptable. > Is there any use case where it is possible to capture from two cameras > at the same time? I think it may simply not be possible on the IPU3 - > but if that's the case, then that should be the documented rationale > behind this patch. From Han-lin's perspective, basically there is no such a use case to enable both cameras at the same time. Maybe it's only true for ipu3. I'm not sure. I also thought that we can dynamically assign ImgUs: if only one camera is being used, it can occupy both the ImgUs. However, when another camera is enabled, the first camera needs to release one of the ImgU, and fall back the StillCapture to VideoSnapshot. Han-lin and I think that ideally it's possible, while we're not sure if it's worth the effort, as there's simply no such a use case, and there's very likely to be some lag during the transition (i.e. the first camera's stream might pause when reconfiguring ISP). > If we could for instance capture from both the IR sensor (which may not > need the ISP?), and the image sensor at the same time on a front facing > camera device like on the SGo2 - then I don't think we should be > preventing that here. IIUC the ipu3 pipeline handler prevents cases that use only StreamRole::Raw, which means the ISP is necessary. Right? -------------- For other suggestions, I'll fix them in the following patch. Let's wait for others' comments on all the patches to prevent the spam :) Thanks for looking into this! BR, Harvey On Thu, May 19, 2022 at 11:01 PM Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Quoting Harvey Yang via libcamera-devel (2022-05-12 11:32:52) > > 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 > > process frames from one single camera. > > I think 'not having many usecases' for something isn't a good reason to > forcibly disable it. If the hardware *can not* do it, then that's > acceptable. > > Is there any use case where it is possible to capture from two cameras > at the same time? I think it may simply not be possible on the IPU3 - > but if that's the case, then that should be the documented rationale > behind this patch. > > If we could for instance capture from both the IR sensor (which may not > need the ISP?), and the image sensor at the same time on a front facing > camera device like on the SGo2 - then I don't think we should be > preventing that here. > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp > b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index fd989e61..111ba053 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,10 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera) > > > > int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const > ControlList *controls) > > { > > + // Deny second camera being started. > > libcamera code style here for a single line comment should be > /* Deny second camera being started. */ > > But what about the third? > > If this is /not/ possible - then I think it should be more like: > > /* > * Enforce that only a single camera can be used at a time due > * to the limitations of the IPU3 IMGU which can only .. > * <detailed of limitation here>. > */ > > > + if (inUseCamera_ && inUseCamera_ != camera) > > + return -1; > > Errnos rather than -1 ... > > Perhaps -EBUSY ... > > > + > > IPU3CameraData *data = cameraData(camera); > > CIO2Device *cio2 = &data->cio2_; > > ImgUDevice *imgu = data->imgu_; > > @@ -781,6 +787,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 +816,8 @@ error: > > freeBuffers(camera); > > LOG(IPU3, Error) << "Failed to start camera " << camera->id(); > > > > + inUseCamera_ = nullptr; > > + > > return ret; > > } > > > > @@ -826,6 +836,8 @@ void PipelineHandlerIPU3::stopDevice(Camera *camera) > > LOG(IPU3, Warning) << "Failed to stop camera " << > camera->id(); > > > > freeBuffers(camera); > > + > > + inUseCamera_ = nullptr; > > } > > > > void IPU3CameraData::cancelPendingRequests() > > -- > > 2.36.0.512.ge40c2bad7a-goog > > >
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index fd989e61..111ba053 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,10 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera) int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlList *controls) { + // Deny second camera being started. + if (inUseCamera_ && inUseCamera_ != camera) + return -1; + IPU3CameraData *data = cameraData(camera); CIO2Device *cio2 = &data->cio2_; ImgUDevice *imgu = data->imgu_; @@ -781,6 +787,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 +816,8 @@ error: freeBuffers(camera); LOG(IPU3, Error) << "Failed to start camera " << camera->id(); + inUseCamera_ = nullptr; + return ret; } @@ -826,6 +836,8 @@ void PipelineHandlerIPU3::stopDevice(Camera *camera) LOG(IPU3, Warning) << "Failed to stop camera " << camera->id(); freeBuffers(camera); + + inUseCamera_ = nullptr; } void IPU3CameraData::cancelPendingRequests()