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