<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>