[libcamera-devel] [PATCH v3 3/9] ipu3: Use imgu0 as default
Cheng-Hao Yang
chenghaoyang at chromium.org
Tue Aug 2 12:30:28 CEST 2022
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 at 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 at 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 at 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)
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220802/465618ac/attachment.htm>
More information about the libcamera-devel
mailing list