[libcamera-devel] [PATCH 03/10] libcamera: ipu3: Initialize and link ImgU devices

[Laurent Pinchart]

Hi Jacopo,

On Sat, Mar 09, 2019 at 09:50:18PM +0100, Jacopo Mondi wrote:
> On Sat, Mar 02, 2019 at 11:41:29PM +0200, Laurent Pinchart wrote:
> > On Thu, Feb 28, 2019 at 09:04:03PM +0100, Jacopo Mondi wrote:
> >> Create video devices and subdevices associated with an ImgU unit, and
> >> link the entities in the media graph to prepare the device for capture
> >> operations at stream configuration time.
> >>
> >> As we support a single stream at the moment, always select imgu0.
> >>
> >> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >> ---
> >>  src/libcamera/pipeline/ipu3/ipu3.cpp | 219 +++++++++++++++++++++++++--
> >>  1 file changed, 207 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> index 4f1ab72debf8..9fa59c1bc97e 100644
> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> @@ -24,6 +24,28 @@ namespace libcamera {
> >>
> >>  LOG_DEFINE_CATEGORY(IPU3)
> >>
> >> +struct ImguDevice {
> >
> > s/ImguDevice/ImgUDevice/ ?
> 
> I would like ImguDevice best, but I'll use ImgUDevice for consistency
> with the kernel documentation.
> 
> >> +	ImguDevice()
> >> +		: imgu(nullptr), input(nullptr), output(nullptr),
> >> +		  viewfinder(nullptr), stat(nullptr) {}
> >
> > 	{
> > 	}
> >
> >> +
> >> +	~ImguDevice()
> >> +	{
> >> +		delete imgu;
> >> +		delete input;
> >> +		delete output;
> >> +		delete viewfinder;
> >> +		delete stat;
> >> +	}
> >
> > Unlike for the CIO2Device, this could be made a class, as I expect it
> > will have more code associated with it, and the ImgU instances will be
> > shared between the multiple camera instances. The linkImgu function
> > would be a good candidate.
> 
> I'll move that to this class. I will has well add a "disableLinks()"
> methods as you suggested below.
> 
> >> +
> >> +	V4L2Subdevice *imgu;
> >> +	V4L2Device *input;
> >> +	V4L2Device *output;
> >> +	V4L2Device *viewfinder;
> >> +	V4L2Device *stat;
> >> +	/* TODO: add param video device for 3A tuning */
> >
> > \todo for consistency, even if this isn't a doxygen comment ?
> 
> Will change all of these.
> 
> >> +};
> >> +
> >>  struct Cio2Device {
> >>  	Cio2Device()
> >>  		: output(nullptr), csi2(nullptr), sensor(nullptr) {}
> >> @@ -44,6 +66,7 @@ class IPU3CameraData : public CameraData
> >>  {
> >>  public:
> >>  	Cio2Device cio2;
> >> +	ImguDevice *imgu;
> >>
> >>  	Stream stream_;
> >>  };
> >> @@ -71,6 +94,10 @@ public:
> >>  	bool match(DeviceEnumerator *enumerator);
> >>
> >>  private:
> >> +	static constexpr unsigned int IMGU_PAD_INPUT = 0;
> >> +	static constexpr unsigned int IMGU_PAD_OUTPUT = 2;
> >> +	static constexpr unsigned int IMGU_PAD_VF = 3;
> >> +	static constexpr unsigned int IMGU_PAD_STAT = 4;
> >>  	static constexpr unsigned int IPU3_BUF_NUM = 4;
> >
> > I'd move that to the ImgUDevice struct/class. You can then remove the
> > IMGU_ prefix.
> 
> Agreed.
> 
> >>
> >>  	IPU3CameraData *cameraData(const Camera *camera)
> >> @@ -79,9 +106,17 @@ private:
> >>  			PipelineHandler::cameraData(camera));
> >>  	}
> >>
> >> +	int linkImgu(ImguDevice *imgu);
> >> +
> >> +	V4L2Device *openDevice(MediaDevice *media, std::string &name);
> >> +	V4L2Subdevice *openSubdevice(MediaDevice *media, std::string &name);
> >> +	int initImgu(ImguDevice *imgu);
> >>  	int initCio2(unsigned int index, Cio2Device *cio2);
> >>  	void registerCameras();
> >>
> >> +	ImguDevice imgu0_;
> >> +	ImguDevice imgu1_;
> >
> > Maybe an array with two entries ?
> 
> They're two, and will stay two, but yes, I'll see how it looks.
> 
> >> +
> >>  	std::shared_ptr<MediaDevice> cio2MediaDev_;
> >>  	std::shared_ptr<MediaDevice> imguMediaDev_;
> >>  };
> >> @@ -159,6 +194,15 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
> >>  	V4L2DeviceFormat devFormat = {};
> >>  	int ret;
> >>
> >> +	/*
> >> +	 * TODO: dynamically assign ImgU devices; as of now, with a single
> >> +	 * stream supported, always use 'imgu0'.
> >
> > 	/**
> > 	 * \todo
> >
> > ?
> >
> >> +	 */
> >> +	data->imgu = &imgu0_;
> >
> > How about moving this to camera creation time, and assigned imgu0 to the
> > first sensor and imgu1 to the second one ?
> >
> 
> I've put it here, as I assumed ImgU association with Cameras will
> depend on the number of required streams. If you confirm my
> understanding I would keep it here instead of moving it back and
> forth.

Your understanding is correct, but with this implementation imgu0 is
used for both cameras, so we can only use one camera at a time. If we
assign imgu0 to the first camera and imgu1 to the second one we can
start using both cameras, with up to two streams each, and later expand
that to more streams.

> >> +	ret = linkImgu(data->imgu);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >>  	/*
> >>  	 * FIXME: as of now, the format gets applied to the sensor and is
> >>  	 * propagated along the pipeline. It should instead be applied on the
> >> @@ -334,17 +378,29 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> >>  	if (cio2MediaDev_->open())
> >>  		goto error_release_mdev;
> >>
> >> +	if (imguMediaDev_->open())
> >> +		goto error_close_mdev;
> >> +
> >>  	if (cio2MediaDev_->disableLinks())
> >> -		goto error_close_cio2;
> >> +		goto error_close_mdev;
> >> +
> >> +	if (initImgu(&imgu0_))
> >> +		goto error_close_mdev;
> >> +
> >> +	if (initImgu(&imgu1_))
> >> +		goto error_close_mdev;
> >> +
> >>
> >>  	registerCameras();
> >>
> >>  	cio2MediaDev_->close();
> >> +	imguMediaDev_->close();
> >>
> >>  	return true;
> >>
> >> -error_close_cio2:
> >> +error_close_mdev:
> >>  	cio2MediaDev_->close();
> >> +	imguMediaDev_->close();
> >
> > Error handling will be simplified when you'll rebase. I'd go as far as
> > calling close() in the PipelineHandlerIPU3 destructor and remove the
> > error path here.
> 
> It has been simplified by rebase, yes. I'll see how moving close() in
> destructor looks like (I think it's good)
> 
> >>
> >>  error_release_mdev:
> >>  	cio2MediaDev_->release();
> >> @@ -353,6 +409,153 @@ error_release_mdev:
> >>  	return false;
> >>  }
> >>
> >> +/* Link entities in the ImgU unit to prepare for capture operations. */
> >> +int PipelineHandlerIPU3::linkImgu(ImguDevice *imguDevice)
> >> +{
> >> +	MediaLink *link;
> >> +	int ret;
> >> +
> >> +	unsigned int index = imguDevice == &imgu0_ ? 0 : 1;
> >> +	std::string imguName = "ipu3-imgu " + std::to_string(index);
> >> +	std::string inputName = imguName + " input";
> >> +	std::string outputName = imguName + " output";
> >> +	std::string viewfinderName = imguName + " viewfinder";
> >> +	std::string statName = imguName + " 3a stat";
> >> +
> >> +	ret = imguMediaDev_->open();
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = imguMediaDev_->disableLinks();
> >
> > This isn't a good idea, as you will interfere with the other ImgU. You
> > should enable/disable links selectively based on your needs.
> 
> As I've said, I'll make a per-imgu instance link disabling method. I
> hope it is enough to disable all links in an imgu instance and we
> don't have to go link by link...

I haven't checked recently, are the two ImgUs exposed as separate media
devices ? If so that's fine, otherwise you'll have to go link by link.

> >> +	if (ret) {
> >> +		imguMediaDev_->close();
> >> +		return ret;
> >> +	}
> >> +
> >> +	/* Link entities to configure the IMGU unit for capture. */
> >> +	link = imguMediaDev_->link(inputName, 0, imguName, IMGU_PAD_INPUT);
> >> +	if (!link) {
> >> +		LOG(IPU3, Error)
> >> +			<< "Failed to get link '" << inputName << "':0 -> '"
> >> +			<< imguName << "':0";
> >
> > Ideally you should use the IMGU_PAD_* constants instead of hardcoding
> > them in error messages.
> 
> Ah well, it's just more lines for a single printout, but ok.
> 
> >> +		ret = -ENODEV;
> >> +		goto error_close_mediadev;
> >> +	}
> >> +	link->setEnabled(true);
> >> +
> >> +	link = imguMediaDev_->link(imguName, IMGU_PAD_OUTPUT, outputName, 0);
> >> +	if (!link) {
> >> +		LOG(IPU3, Error)
> >> +			<< "Failed to get link '" << imguName << "':2 -> '"
> >> +			<< outputName << "':0";
> >> +		ret = -ENODEV;
> >> +		goto error_close_mediadev;
> >> +	}
> >> +	link->setEnabled(true);
> >> +
> >> +	link = imguMediaDev_->link(imguName, IMGU_PAD_VF, viewfinderName, 0);
> >> +	if (!link) {
> >> +		LOG(IPU3, Error)
> >> +			<< "Failed to get link '" << imguName << "':3 -> '"
> >> +			<< viewfinderName << "':0";
> >> +		ret = -ENODEV;
> >> +		goto error_close_mediadev;
> >> +	}
> >> +	link->setEnabled(true);
> >
> > We have a single stream, why do you enable both output and viewfinder ?
> 
> From my testings, if I keep them disabled (== not linked) the system
> hangs even if I'm only capturing from the ouput video device. I will
> do some more testing, as setting up those nodes, queuing and dequeuing
> buffers there requires some not-nice code to keep track of the buffer
> indexes, as you've noticed.

Another kernel bug fix candidate :-)

> >> +
> >> +	link = imguMediaDev_->link(imguName, IMGU_PAD_STAT, statName, 0);
> >> +	if (!link) {
> >> +		LOG(IPU3, Error)
> >> +			<< "Failed to get link '" << imguName << "':4 -> '"
> >> +			<< statName << "':0";
> >> +		ret = -ENODEV;
> >> +		goto error_close_mediadev;
> >> +	}
> >> +	link->setEnabled(true);
> >
> > Same here, we don't make use of stats yes, there's no need to enable
> > this link.
> >
> >> +
> >> +	imguMediaDev_->close();
> >> +
> >> +	return 0;
> >> +
> >> +error_close_mediadev:
> >> +	imguMediaDev_->close();
> >
> > We really really need to think about how to handle open/close and write
> > down a set of rules. Please make sure this is captured in a \todo.
> >
> >> +
> >> +	return ret;
> >> +
> >> +}
> >> +
> >> +V4L2Device *PipelineHandlerIPU3::openDevice(MediaDevice *media,
> >> +					    std::string &name)
> >
> > const std::string &
> >
> > Same below.
> >
> >> +{
> >> +	V4L2Device *dev;
> >
> > You can move this line down to declare and initialize the variable at
> > the same time.
> >
> >> +
> >> +	MediaEntity *entity = media->getEntityByName(name);
> >> +	if (!entity) {
> >> +		LOG(IPU3, Error)
> >> +			<< "Failed to get entity '" << name << "'";
> >> +		return nullptr;
> >> +	}
> >> +
> >> +	dev = new V4L2Device(entity);
> >> +	if (dev->open())
> >> +		return nullptr;
> >
> > You'll leak dev, a delete is needed.
> 
> Ah, right! will fix.
> 
> >> +
> >> +	return dev;
> >> +}
> >> +
> >> +V4L2Subdevice *PipelineHandlerIPU3::openSubdevice(MediaDevice *media,
> >> +						  std::string &name)
> >> +{
> >> +	V4L2Subdevice *dev;
> >> +
> >> +	MediaEntity *entity = media->getEntityByName(name);
> >> +	if (!entity) {
> >> +		LOG(IPU3, Error)
> >> +			<< "Failed to get entity '" << name << "'";
> >> +		return nullptr;
> >> +	}
> >> +
> >> +	dev = new V4L2Subdevice(entity);
> >> +	if (dev->open())
> >
> > Leak.
> >
> >> +		return nullptr;
> >> +
> >> +	return dev;
> >> +}
> >
> > These two functions may be candidates for future generic helpers. Could
> > you document them and add a \todo to mention this ?
> 
> Agreed!
> 
> >> +
> >> +/* Create video devices and subdevices for the ImgU instance. */
> >> +int PipelineHandlerIPU3::initImgu(ImguDevice *imgu)
> >> +{
> >> +	unsigned int index = imgu == &imgu0_ ? 0 : 1;
> >
> > A bit ugly, how about adding an index field to the ImguDevice structure
> > ?
> 
> Yes. But when would you intialize such a field? at match time? In this
> function? I'll see how it'll look

I think I'd do it at match time.

> >> +	std::string imguName = "ipu3-imgu " + std::to_string(index);
> >> +	std::string devName;
> >> +
> >> +	imgu->imgu = openSubdevice(imguMediaDev_.get(), imguName);
> >> +	if (!imgu->imgu)
> >> +		return -ENODEV;
> >> +
> >> +	devName = imguName + " input";
> >> +	imgu->input = openDevice(imguMediaDev_.get(), devName);
> >> +	if (!imgu->input)
> >> +		return -ENODEV;
> >> +
> >> +	devName = imguName + " output";
> >> +	imgu->output = openDevice(imguMediaDev_.get(), devName);
> >> +	if (!imgu->output)
> >> +		return -ENODEV;
> >> +
> >> +	devName = imguName + " viewfinder";
> >> +	imgu->viewfinder = openDevice(imguMediaDev_.get(), devName);
> >> +	if (!imgu->viewfinder)
> >> +		return -ENODEV;
> >> +
> >> +	devName = imguName + " 3a stat";
> >> +	imgu->stat = openDevice(imguMediaDev_.get(), devName);
> >
> > You could drop devName and call
> >
> > 	imgu->state = openDevice(imguMediaDev_.get(), imguName + " 3a stat");
> >
> > Up to you.
> 
> Less lines, so it's better
> 
> >> +	if (!imgu->stat)
> >> +		return -ENODEV;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2)
> >>  {
> >>  	int ret;
> >> @@ -400,16 +603,8 @@ int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2)
> >>  		return ret;
> >>
> >>  	std::string cio2Name = "ipu3-cio2 " + std::to_string(index);
> >> -	entity = cio2MediaDev_->getEntityByName(cio2Name);
> >> -	if (!entity) {
> >> -		LOG(IPU3, Error)
> >> -			<< "Failed to get entity '" << cio2Name << "'";
> >> -		return -EINVAL;
> >> -	}
> >> -
> >> -	cio2->output = new V4L2Device(entity);
> >> -	ret = cio2->output->open();
> >> -	if (ret)
> >> +	cio2->output = openDevice(cio2MediaDev_.get(), cio2Name);
> >> +	if (!cio2->output)
> >>  		return ret;
> >>
> >>  	return 0;

-- 
Regards,

Laurent Pinchart