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