[libcamera-devel] [PATCH v4 06/31] libcamera: ipu3: Initialize and configure ImgUs

[Jacopo Mondi]

Ups, self correction...

On Mon, Mar 25, 2019 at 10:43:06AM +0100, Jacopo Mondi wrote:
> Hi Laurent,
>

[snip]

> > > +	return 0;
> > > +
> > > +error_delete_vf:
> > > +	delete imgu->viewfinder;
> > > +error_delete_output:
> > > +	delete imgu->output;
> > > +error_delete_input:
> > > +	delete imgu->input;
> > > +error_delete_imgu:
> > > +	delete imgu->imgu;
> >
> > Let's simplify this by removing the need for this code. The caller can
> > just propagate the error up, the match function will fail, and the imgu
> > instances will be destroyed.
> >
>
> From your comment I get that we can fail when intializing the ImgUs,
> propagate the error up and make PipelineHandler::match() fail, which
> deletes the pipeline handler instance and consequentially the ImgU
> instances.
>
> Just to make sure we're on the same page:
> - the pipeline manager instances are destroyed at
>   CameraManager::stop() time only
> - if PipelineHandler::match() fails, the pipeline handler is not
>   destroyed.

It is, as it is created as shared pointer with a single reference by
PipelineHandlerFactory::create().

Sorry for the noise, I had missed that the one created at match() time
is the single reference to the newly created pipeline handler.

Thanks for pointing me to that.


> - It is not enough to return an error from the
>   PipelineHandler::match() function to have the ImgU initialized instances
>   (if any) deleted
>
> It is very likely we can wait for CameraManager::stop() and have the
> ImgU instances destroyed there, but I wonder if it would not be worth
> cleaning them up as soon as we fail. The easies way would be
> instanciate them at initialization time, and delete the newly created
> instances if one of the two initializations fail, or associate them
> with camera data as we do for CIO2Device instances, but then your
> comment to initialize ImgUs separately from camera creation won't
> apply anymore.
>
> My proposal would be something like:
>
> class ImgUDevice
> {
>         ...
>
>         ImgUDevice *imgu0_;
>         ImgUDevice *imgu1_;
> }
>
> PipelineHandler::match()
> {
>
>
>         ret = registerCameras();
>         if (ret) {
>                 closeMediaDevices
>                 return false;
>         }
> }
>
> PipelineHandler::registerCameras()
> {
>
>         imgu0_ = new ImgUDevice();
>         ret = imgu0_->init();
>         if (ret) {
>                 delete imgu0_;
>                 return -ENODEV;
>         }
>
>         imgu1_ = new ImgUDevice();
>         ret = imgu1_->init();
>         if (ret) {
>                 delete imgu0_;
>                 delete imgu1_;
>                 return -ENODEV;
>         }
>
>         for (i = 0 ; i < 4; ++i) {
>                 ret = cio2::init();
>                 if (ret)
>                         continue;
>
>                 registerCamera();
>                 ++numCameras;
>         }
>
>         if (!numCameras) {
>                 delete imgu0_;
>                 delete imgu1_;
>
>                 return -ENODEV;
>        }
>
>        return 0;
> }
>
> Opinions?
>
> Thanks
>   j
>
> > > +
> > > +	return -ENODEV;
> > > +}
> > > +
> > >  /**
> > >   * \brief Initialize components of the CIO2 device \a index used by a camera
> > >   * \param index The CIO2 device index
> > > @@ -458,23 +642,12 @@ int PipelineHandlerIPU3::initCIO2(unsigned int index, CIO2Device *cio2)
> > >  	if (ret)
> > >  		goto error_delete_csi2;
> > >
> > > -	entity = cio2MediaDev_->getEntityByName(cio2Name);
> > > -	if (!entity) {
> > > -		LOG(IPU3, Error)
> > > -			<< "Failed to get entity '" << cio2Name << "'";
> > > -		ret = -EINVAL;
> > > +	cio2->output = openDevice(cio2MediaDev_.get(), cio2Name);
> > > +	if (!cio2->output)
> > >  		goto error_delete_csi2;
> > > -	}
> > > -
> > > -	cio2->output = new V4L2Device(entity);
> > > -	ret = cio2->output->open();
> > > -	if (ret)
> > > -		goto error_delete_output;
> > >
> > >  	return 0;
> > >
> > > -error_delete_output:
> > > -	delete cio2->output;
> > >  error_delete_csi2:
> > >  	delete cio2->csi2;
> > >  error_delete_sensor:
> > > @@ -483,6 +656,16 @@ error_delete_sensor:
> > >  	return ret;
> > >  }
> > >
> > > +/**
> > > + * \brief Delete all devices associated with a CIO2 unit
> > > + */
> > > +void PipelineHandlerIPU3::deleteCIO2(CIO2Device *cio2)
> > > +{
> > > +	delete cio2->output;
> > > +	delete cio2->csi2;
> > > +	delete cio2->sensor;
> > > +}
> >
> > I don't think this is needed for the reason explained above.
> >
> > > +
> > >  /*
> > >   * Cameras are created associating an image sensor (represented by a
> > >   * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four
> > > @@ -495,7 +678,7 @@ void PipelineHandlerIPU3::registerCameras()
> > >  	 * image sensor is connected to it.
> > >  	 */
> > >  	unsigned int numCameras = 0;
> > > -	for (unsigned int id = 0; id < 4; ++id) {
> > > +	for (unsigned int id = 0; id < 4 && numCameras < 2; ++id) {
> > >  		std::unique_ptr<IPU3CameraData> data =
> > >  			utils::make_unique<IPU3CameraData>(this);
> > >  		std::set<Stream *> streams{ &data->stream_ };
> > > @@ -506,6 +689,18 @@ void PipelineHandlerIPU3::registerCameras()
> > >  		if (ret)
> > >  			continue;
> > >
> > > +		/**
> > > +		 * \todo Dynamically assign ImgU devices; as of now, limit
> > > +		 * support to two cameras only, and assign imgu0 to the first
> > > +		 * one and imgu1 to the second.
> > > +		 */
> > > +		data->imgu = &imgus_[numCameras];
> > > +		ret = initImgU(data->imgu);
> >
> > The imgus are separate from the cameras, please initialize them before
> > this loop, and return an error if initialization fails.
> >
> > > +		if (ret) {
> > > +			deleteCIO2(cio2);
> > > +			continue;
> > > +		}
> > > +
> > >  		std::string cameraName = cio2->sensor->deviceName() + " "
> > >  				       + std::to_string(id);
> > >  		std::shared_ptr<Camera> camera = Camera::create(this,
> >
> > --
> > Regards,
> >
> > Laurent Pinchart



> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190325/efa0266b/attachment.sig>