[libcamera-devel] [PATCH 12/13] libcamera: ipu3: Align how CIO2 and ImgU are stored in CameraData
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Jun 27 18:48:34 CEST 2020
Hi Niklas,
Thank you for the patch.
On Sat, Jun 27, 2020 at 12:43:33PM +0200, Niklas Söderlund wrote:
> 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.
Correct, we thought we could use two ImgU instances for a single camera
(and we likely will need to do so in the future). I also agree that more
changes will be needed.
> > > 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_;
I'm not sure to like this much though. For all it's worth, I would have
gone the other way around, make cio2_ a pointer too. There are 4 CIO2
devices, and we could thus support more than two cameras, provided
they're not all used at the same time. It would make sense to
instantiate 4 CIO2 devices in PipelineHandlerIPU3, and point to the
corresponding CIO2 in IPU3CameraData. That's most likely what we will
end up doing, and if we embed both the CIO2 and IMGU in IPU3CameraData
we will then make more code changes that will rely on that design,
making future cleanups more difficult. I also don't really see this
patch performing any change that allows further simplifications in the
rest of the series (as far as I can tell, 13/13 doesn't depend on it).
I'd thus rather keep the current design, and if you want to unify the
cio2_ and imgu_ field, replace the patch with one that would turn cio2_
into a pointer.
> > > 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);
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list