<div dir="ltr"><div>Hi Umang,</div><div><br></div><div>Thanks for your review!</div><div>I guess there are only the three comments in your third message that need updates, right?<br>Please check if I miss anything.</div><div dir="ltr"></div><div><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jul 27, 2022 at 3:38 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 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>
> With only one camera being started, we can always use imgu0 to process<br>
> frames (for video/preview). In the following patches, we'll use imgu1<br>
> for still capture if needed.<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 | 86 ++++++++++++++++------------<br>
> 1 file changed, 48 insertions(+), 38 deletions(-)<br>
><br>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> index c943ee6a..e219f704 100644<br>
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> @@ -64,7 +64,8 @@ public:<br>
> void frameStart(uint32_t sequence);<br>
> <br>
> CIO2Device cio2_;<br>
> - ImgUDevice *imgu_;<br>
> + ImgUDevice *imgu0_;<br>
> + ImgUDevice *imgu1_;<br>
> <br>
> Stream outStream_;<br>
> Stream vfStream_;<br>
> @@ -406,7 +407,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()<br>
> <br>
> /* Only compute the ImgU configuration if a YUV stream has been requested. */<br>
> if (yuvCount) {<br>
> - pipeConfig_ = data_->imgu_->calculatePipeConfig(&pipe);<br>
> + pipeConfig_ = data_->imgu0_->calculatePipeConfig(&pipe);<br>
> if (pipeConfig_.isNull()) {<br>
> LOG(IPU3, Error) << "Failed to calculate pipe configuration: "<br>
> << "unsupported resolutions.";<br>
> @@ -518,7 +519,6 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)<br>
> Stream *outStream = &data->outStream_;<br>
> Stream *vfStream = &data->vfStream_;<br>
> CIO2Device *cio2 = &data->cio2_;<br>
> - ImgUDevice *imgu = data->imgu_;<br>
> V4L2DeviceFormat outputFormat;<br>
> int ret;<br>
> <br>
> @@ -560,7 +560,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)<br>
<br>
<br>
We need to update the ::configure() comment about multiple camera <br>
streaming (listed as FIXME) , since we will only stream one camera at a <br>
given time now.<br>
<br></blockquote><div><br></div><div>Updated the comment, not sure if I'm doing it right. Please check :)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> * stream which is for raw capture, in which case no buffers will<br>
> * ever be queued to the ImgU.<br>
> */<br>
> - ret = data->imgu_->enableLinks(true);<br>
> + ret = imgu0_.enableLinks(true);<br>
> if (ret)<br>
> return ret;<br>
> <br>
> @@ -610,7 +610,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)<br>
> if (imguConfig.isNull())<br>
> return 0;<br>
> <br>
> - ret = imgu->configure(imguConfig, &cio2Format);<br>
> + ret = imgu0_.configure(imguConfig, &cio2Format);<br>
> if (ret)<br>
> return ret;<br>
> <br>
> @@ -624,12 +624,12 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)<br>
> <br>
> if (stream == outStream) {<br>
> mainCfg = &cfg;<br>
> - ret = imgu->configureOutput(cfg, &outputFormat);<br>
> + ret = imgu0_.configureOutput(cfg, &outputFormat);<br>
> if (ret)<br>
> return ret;<br>
> } else if (stream == vfStream) {<br>
> vfCfg = &cfg;<br>
> - ret = imgu->configureViewfinder(cfg, &outputFormat);<br>
> + ret = imgu0_.configureViewfinder(cfg, &outputFormat);<br>
> if (ret)<br>
> return ret;<br>
> }<br>
> @@ -641,13 +641,13 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)<br>
> * be at least one active stream in the configuration request).<br>
> */<br>
> if (!vfCfg) {<br>
> - ret = imgu->configureViewfinder(*mainCfg, &outputFormat);<br>
> + ret = imgu0_.configureViewfinder(*mainCfg, &outputFormat);<br>
> if (ret)<br>
> return ret;<br>
> }<br>
> <br>
> /* Apply the "pipe_mode" control to the ImgU subdevice. */<br>
> - ControlList ctrls(imgu->imgu_->controls());<br>
> + ControlList ctrls(imgu0_.imgu_->controls());<br>
> /*<br>
> * Set the ImgU pipe mode to 'Video' unconditionally to have statistics<br>
> * generated.<br>
> @@ -657,7 +657,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)<br>
> */<br>
> ctrls.set(V4L2_CID_IPU3_PIPE_MODE,<br>
> static_cast<int32_t>(IPU3PipeModeVideo));<br>
> - ret = imgu->imgu_->setControls(&ctrls);<br>
> + ret = imgu0_.imgu_->setControls(&ctrls);<br>
> if (ret) {<br>
> LOG(IPU3, Error) << "Unable to set pipe_mode control";<br>
> return ret;<br>
> @@ -691,9 +691,9 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,<br>
> unsigned int count = stream->configuration().bufferCount;<br>
> <br>
> if (stream == &data->outStream_)<br>
> - return data->imgu_->output_->exportBuffers(count, buffers);<br>
> + return imgu0_.output_->exportBuffers(count, buffers);<br>
> else if (stream == &data->vfStream_)<br>
> - return data->imgu_->viewfinder_->exportBuffers(count, buffers);<br>
> + return imgu0_.viewfinder_->exportBuffers(count, buffers);<br>
> else if (stream == &data->rawStream_)<br>
> return data->cio2_.exportBuffers(count, buffers);<br>
> <br>
> @@ -711,7 +711,6 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,<br>
> int PipelineHandlerIPU3::allocateBuffers(Camera *camera)<br>
> {<br>
> IPU3CameraData *data = cameraData(camera);<br>
> - ImgUDevice *imgu = data->imgu_;<br>
> unsigned int bufferCount;<br>
> int ret;<br>
> <br>
> @@ -721,26 +720,26 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)<br>
> data->rawStream_.configuration().bufferCount,<br>
> });<br>
> <br>
> - ret = imgu->allocateBuffers(bufferCount);<br>
> + ret = imgu0_.allocateBuffers(bufferCount);<br>
> if (ret < 0)<br>
> return ret;<br>
> <br>
> /* Map buffers to the IPA. */<br>
> unsigned int ipaBufferId = 1;<br>
> <br>
> - for (const std::unique_ptr<FrameBuffer> &buffer : imgu->paramBuffers_) {<br>
> + for (const std::unique_ptr<FrameBuffer> &buffer : imgu0_.paramBuffers_) {<br>
> buffer->setCookie(ipaBufferId++);<br>
> ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());<br>
> }<br>
> <br>
> - for (const std::unique_ptr<FrameBuffer> &buffer : imgu->statBuffers_) {<br>
> + for (const std::unique_ptr<FrameBuffer> &buffer : imgu0_.statBuffers_) {<br>
> buffer->setCookie(ipaBufferId++);<br>
> ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());<br>
> }<br>
> <br>
> data->ipa_->mapBuffers(ipaBuffers_);<br>
> <br>
> - data->frameInfos_.init(imgu->paramBuffers_, imgu->statBuffers_);<br>
> + data->frameInfos_.init(imgu0_.paramBuffers_, imgu0_.statBuffers_);<br>
> data->frameInfos_.bufferAvailable.connect(<br>
> data, &IPU3CameraData::queuePendingRequests);<br>
> <br>
> @@ -760,7 +759,7 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)<br>
> data->ipa_->unmapBuffers(ids);<br>
> ipaBuffers_.clear();<br>
> <br>
> - data->imgu_->freeBuffers();<br>
> + imgu0_.freeBuffers();<br>
> <br>
> return 0;<br>
> }<br>
> @@ -777,9 +776,18 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis<br>
> <br>
> IPU3CameraData *data = cameraData(camera);<br>
> CIO2Device *cio2 = &data->cio2_;<br>
> - ImgUDevice *imgu = data->imgu_;<br>
> int ret;<br>
> <br>
> + imgu0_.input_->bufferReady.connect(&data->cio2_,<br>
> + &CIO2Device::tryReturnBuffer);<br>
> + imgu0_.output_->bufferReady.connect(data,<br>
> + &IPU3CameraData::imguOutputBufferReady);<br>
> + imgu0_.viewfinder_->bufferReady.connect(data,<br>
> + &IPU3CameraData::imguOutputBufferReady);<br>
> + imgu0_.param_->bufferReady.connect(data,<br>
> + &IPU3CameraData::paramBufferReady);<br>
> + imgu0_.stat_->bufferReady.connect(data,<br>
> + &IPU3CameraData::statBufferReady);<br>
> /* Disable test pattern mode on the sensor, if any. */<br>
> ret = cio2->sensor()->setTestPatternMode(<br>
> controls::draft::TestPatternModeEnum::TestPatternModeOff);<br>
> @@ -807,19 +815,24 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis<br>
> if (ret)<br>
> goto error;<br>
> <br>
> - ret = imgu->start();<br>
> + ret = imgu0_.start();<br>
> if (ret)<br>
> goto error;<br>
> <br>
> return 0;<br>
> <br>
> error:<br>
> - imgu->stop();<br>
> + imgu0_.stop();<br>
> cio2->stop();<br>
> data->ipa_->stop();<br>
> freeBuffers(camera);<br>
> LOG(IPU3, Error) << "Failed to start camera " << camera->id();<br>
> <br>
> + imgu0_.input_->bufferReady.disconnect();<br>
> + imgu0_.output_->bufferReady.disconnect();<br>
> + imgu0_.viewfinder_->bufferReady.disconnect();<br>
> + imgu0_.param_->bufferReady.disconnect();<br>
> + imgu0_.stat_->bufferReady.disconnect();<br>
> inUseCamera_ = nullptr;<br>
> <br>
> return ret;<br>
> @@ -834,13 +847,19 @@ void PipelineHandlerIPU3::stopDevice(Camera *camera)<br>
> <br>
> data->ipa_->stop();<br>
> <br>
> - ret |= data->imgu_->stop();<br>
> + ret |= imgu0_.stop();<br>
> ret |= data->cio2_.stop();<br>
> if (ret)<br>
> LOG(IPU3, Warning) << "Failed to stop camera " << camera->id();<br>
> <br>
> freeBuffers(camera);<br>
> <br>
> + data->imgu0_->input_->bufferReady.disconnect();<br>
> + data->imgu0_->output_->bufferReady.disconnect();<br>
> + data->imgu0_->viewfinder_->bufferReady.disconnect();<br>
> + data->imgu0_->param_->bufferReady.disconnect();<br>
> + data->imgu0_->stat_->bufferReady.disconnect();<br>
> +<br>
<br>
<br>
I think we should better abstract the signal connections/disconnections <br>
to IMGU somehow, if we are going to use them in ::start().<br>
<br>
Probably the abstractions can come through IPU3CameraData ?<br>
<br></blockquote><div><br></div><div>For connections, |input_->bufferReady| is bound to CIO2Device instead, and we need to bind different methods on |imgu1_|'s signals, when StillCapture is enabled.</div><div>I don't think there's an elegant way to abstract it...? </div><div><br></div><div>For disconnections, I think adding a helper function in ImgUDevice should be simpler. WDYT? (Added in the next version)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> inUseCamera_ = nullptr;<br>
> }<br>
> <br>
> @@ -1184,7 +1203,8 @@ int PipelineHandlerIPU3::registerCameras()<br>
> * only, and assign imgu0 to the first one and imgu1 to the<br>
> * second.<br>
> */<br>
> - data->imgu_ = numCameras ? &imgu1_ : &imgu0_;<br>
> + data->imgu0_ = &imgu0_;<br>
> + data->imgu1_ = &imgu1_;<br>
> <br>
> /*<br>
> * Connect video devices' 'bufferReady' signals to their<br>
> @@ -1198,16 +1218,6 @@ int PipelineHandlerIPU3::registerCameras()<br>
> &IPU3CameraData::cio2BufferReady);<br>
> data->cio2_.bufferAvailable.connect(<br>
> data.get(), &IPU3CameraData::queuePendingRequests);<br>
<br>
<br>
How about we move the cio2_ signals connection to <br>
PIpelineHandlerIPU3::Start() as well? There are 2 signals w.r.t cio2_ : <br>
bufferReady and bufferAvailable.<br>
<br></blockquote><div> </div><div>Hmm, but PipelineHandlerIPU3::Start() might be called multiple times, while we only need to setup |cio2_|'s signals connection once. Right?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> - data->imgu_->input_->bufferReady.connect(&data->cio2_,<br>
> - &CIO2Device::tryReturnBuffer);<br>
> - data->imgu_->output_->bufferReady.connect(data.get(),<br>
> - &IPU3CameraData::imguOutputBufferReady);<br>
> - data->imgu_->viewfinder_->bufferReady.connect(data.get(),<br>
> - &IPU3CameraData::imguOutputBufferReady);<br>
> - data->imgu_->param_->bufferReady.connect(data.get(),<br>
> - &IPU3CameraData::paramBufferReady);<br>
> - data->imgu_->stat_->bufferReady.connect(data.get(),<br>
> - &IPU3CameraData::statBufferReady);<br>
> <br>
> /* Create and register the Camera instance. */<br>
> const std::string &cameraId = cio2->sensor()->id();<br>
> @@ -1300,14 +1310,14 @@ void IPU3CameraData::paramsBufferReady(unsigned int id)<br>
> FrameBuffer *outbuffer = it.second;<br>
> <br>
> if (stream == &outStream_)<br>
> - imgu_->output_->queueBuffer(outbuffer);<br>
> + imgu0_->output_->queueBuffer(outbuffer);<br>
> else if (stream == &vfStream_)<br>
> - imgu_->viewfinder_->queueBuffer(outbuffer);<br>
> + imgu0_->viewfinder_->queueBuffer(outbuffer);<br>
> }<br>
> <br>
> - imgu_->param_->queueBuffer(info->paramBuffer);<br>
> - imgu_->stat_->queueBuffer(info->statBuffer);<br>
> - imgu_->input_->queueBuffer(info->rawBuffer);<br>
> + imgu0_->param_->queueBuffer(info->paramBuffer);<br>
> + imgu0_->stat_->queueBuffer(info->statBuffer);<br>
> + imgu0_->input_->queueBuffer(info->rawBuffer);<br>
> }<br>
> <br>
> void IPU3CameraData::metadataReady(unsigned int id, const ControlList &metadata)<br>
</blockquote></div></div>