[libcamera-devel] [PATCH 12/13] libcamera: ipu3: Align how CIO2 and ImgU are stored in CameraData
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sat Jun 27 12:43:33 CEST 2020
Hi Jacopo,
Thanks for your feedback.
On 2020-06-27 12:16:30 +0200, Jacopo Mondi wrote:
> On Sat, Jun 27, 2020 at 05:00:42AM +0200, Niklas Söderlund wrote:
> > One is a pointer and the other is not. It is unintuitive to interact
> > with and with our current design of one ImgU for each camera there is no
> > advantage of having it as a pointer. Our current design is unlikely to
> > change as the system really only has one ImgU. Align the two to make the
> > pipeline more consistent.
>
> Do you recall why we wanted to assign ImgU devices dynamically ?
We had ideas of providing more then 2 streams per camera IIRC. This
might still happen but would likely be quiet an big change to the
pipeline so I see no reason to keep this small part in preparation for
this. Likely this would change in any case if we ever go down that
route.
>
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 48 ++++++++++++----------------
> > 1 file changed, 20 insertions(+), 28 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 0ebd762982142471..6ae4728b470f9487 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -43,7 +43,7 @@ public:
> > void cio2BufferReady(FrameBuffer *buffer);
> >
> > CIO2Device cio2_;
> > - ImgUDevice *imgu_;
> > + ImgUDevice imgu_;
> >
> > Stream outStream_;
> > Stream vfStream_;
> > @@ -117,8 +117,6 @@ private:
> > int allocateBuffers(Camera *camera);
> > int freeBuffers(Camera *camera);
> >
> > - ImgUDevice imgu0_;
> > - ImgUDevice imgu1_;
> > MediaDevice *cio2MediaDev_;
> > MediaDevice *imguMediaDev_;
> > };
> > @@ -414,7 +412,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > Stream *outStream = &data->outStream_;
> > Stream *vfStream = &data->vfStream_;
> > CIO2Device *cio2 = &data->cio2_;
> > - ImgUDevice *imgu = data->imgu_;
> > + ImgUDevice *imgu = &data->imgu_;
> > V4L2DeviceFormat outputFormat;
> > int ret;
> >
> > @@ -456,7 +454,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 = data->imgu_.enableLinks(true);
> > if (ret)
> > return ret;
> >
> > @@ -563,9 +561,9 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
> > unsigned int count = stream->configuration().bufferCount;
> >
> > if (stream == &data->outStream_)
> > - return data->imgu_->output_.dev->exportBuffers(count, buffers);
> > + return data->imgu_.output_.dev->exportBuffers(count, buffers);
> > else if (stream == &data->vfStream_)
> > - return data->imgu_->viewfinder_.dev->exportBuffers(count, buffers);
> > + return data->imgu_.viewfinder_.dev->exportBuffers(count, buffers);
> > else if (stream == &data->rawStream_)
> > return data->cio2_.exportBuffers(count, buffers);
> >
> > @@ -583,7 +581,7 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
> > int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
> > {
> > IPU3CameraData *data = cameraData(camera);
> > - ImgUDevice *imgu = data->imgu_;
> > + ImgUDevice *imgu = &data->imgu_;
> > unsigned int bufferCount;
> > int ret;
> >
> > @@ -604,7 +602,7 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)
> > {
> > IPU3CameraData *data = cameraData(camera);
> >
> > - data->imgu_->freeBuffers();
> > + data->imgu_.freeBuffers();
> >
> > return 0;
> > }
> > @@ -613,7 +611,7 @@ int PipelineHandlerIPU3::start(Camera *camera)
> > {
> > IPU3CameraData *data = cameraData(camera);
> > CIO2Device *cio2 = &data->cio2_;
> > - ImgUDevice *imgu = data->imgu_;
> > + ImgUDevice *imgu = &data->imgu_;
> > int ret;
> >
> > /* Allocate buffers for internal pipeline usage. */
> > @@ -650,7 +648,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> > IPU3CameraData *data = cameraData(camera);
> > int ret = 0;
> >
> > - ret |= data->imgu_->stop();
> > + ret |= data->imgu_.stop();
> > ret |= data->cio2_.stop();
> > if (ret)
> > LOG(IPU3, Warning) << "Failed to stop camera "
> > @@ -680,9 +678,9 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
> > int ret;
> >
> > if (stream == &data->outStream_)
> > - ret = data->imgu_->output_.dev->queueBuffer(buffer);
> > + ret = data->imgu_.output_.dev->queueBuffer(buffer);
> > else if (stream == &data->vfStream_)
> > - ret = data->imgu_->viewfinder_.dev->queueBuffer(buffer);
> > + ret = data->imgu_.viewfinder_.dev->queueBuffer(buffer);
> > else
> > continue;
> >
> > @@ -757,16 +755,6 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> > */
> > int PipelineHandlerIPU3::registerCameras()
> > {
> > - int ret;
> > -
> > - ret = imgu0_.init(imguMediaDev_, 0);
> > - if (ret)
> > - return ret;
> > -
> > - ret = imgu1_.init(imguMediaDev_, 1);
> > - if (ret)
> > - return ret;
> > -
> > /*
> > * For each CSI-2 receiver on the IPU3, create a Camera if an
> > * image sensor is connected to it and the sensor can produce images
> > @@ -774,6 +762,8 @@ int PipelineHandlerIPU3::registerCameras()
> > */
> > unsigned int numCameras = 0;
> > for (unsigned int id = 0; id < 4 && numCameras < 2; ++id) {
> > + int ret;
> > +
> > std::unique_ptr<IPU3CameraData> data =
> > std::make_unique<IPU3CameraData>(this);
> > std::set<Stream *> streams = {
> > @@ -796,7 +786,9 @@ int PipelineHandlerIPU3::registerCameras()
> > * only, and assign imgu0 to the first one and imgu1 to the
> > * second.
> > */
>
> /**
> * \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.
> */
>
> Is this a leftover ? Should you remove it as well?
Good point I will remove it, thanks.
>
> > - data->imgu_ = numCameras ? &imgu1_ : &imgu0_;
> > + ret = data->imgu_.init(imguMediaDev_, numCameras);
> > + if (ret)
> > + return ret;
> >
> > /*
> > * Connect video devices' 'bufferReady' signals to their
> > @@ -808,11 +800,11 @@ int PipelineHandlerIPU3::registerCameras()
> > */
> > data->cio2_.bufferReady.connect(data.get(),
> > &IPU3CameraData::cio2BufferReady);
> > - data->imgu_->input_->bufferReady.connect(&data->cio2_,
> > + data->imgu_.input_->bufferReady.connect(&data->cio2_,
> > &CIO2Device::tryReturnBuffer);
> > - data->imgu_->output_.dev->bufferReady.connect(data.get(),
> > + data->imgu_.output_.dev->bufferReady.connect(data.get(),
> > &IPU3CameraData::imguOutputBufferReady);
> > - data->imgu_->viewfinder_.dev->bufferReady.connect(data.get(),
> > + data->imgu_.viewfinder_.dev->bufferReady.connect(data.get(),
> > &IPU3CameraData::imguOutputBufferReady);
> >
> > /* Create and register the Camera instance. */
> > @@ -881,7 +873,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
> > return;
> > }
> >
> > - imgu_->input_->queueBuffer(buffer);
> > + imgu_.input_->queueBuffer(buffer);
> > }
> >
> > REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
> > --
> > 2.27.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list