[libcamera-devel] [PATCH 12/13] libcamera: ipu3: Align how CIO2 and ImgU are stored in CameraData
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sun Jun 28 01:05:11 CEST 2020
Hi Laurent,
Thanks for your feedback.
On 2020-06-27 19:48:34 +0300, Laurent Pinchart wrote:
> 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.
You are correct that 13/13 does not depend on this. This was something I
spotted while working with the series which felt unnatural that the CIO2
and ImgU where handled differently.
I find it unlikely that we will extend the IPU3 pipeline with support
for more then two cameras before we have at least added an basic IPA and
and sorted out a lot of other things. Likewise I think it will be a
while before we attempt to have a singe camera use more then one ImgU,
if ever. I think code should reflect the current usage and take into
account future planed work which are schedule to happen and not
theoretical use-cases that are not on the horizon :-) I fear this is a
bikesheeding issue and the patch itself does not add or remove much
value from the real work in this series so I will drop it for v2.
>
> > > > 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
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list