[libcamera-devel] [PATCH v3 2/8] libcamera: ipu3: Create camera with 2 streams

[Laurent Pinchart]

Hi Jacopo,

Thank you.

On Mon, Apr 08, 2019 at 09:54:08AM +0200, Jacopo Mondi wrote:
> On Fri, Apr 05, 2019 at 06:44:05PM +0300, Laurent Pinchart wrote:
> > On Wed, Apr 03, 2019 at 05:07:29PM +0200, Jacopo Mondi wrote:
> >> Create each IPU3 camera with two streams: 'output' and 'viewfinder'
> >> which represents the video stream from main and secondary ImgU output
> >> respectively.
> >>
> >> Re-work stream configuration to handle the two video streams 'output'
> >> and 'viewfinder' separately.
> >>
> >> As the IPU3 driver requires viewfinder and stat video nodes to be
> >> started not to stall ImgU processing, keep track of which streams have
> >> been requested by the application, and configure 'output', 'viewfinder'
> >> and 'stat' regardless of the user requests.
> >>
> >> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >> ---
> >>  src/libcamera/pipeline/ipu3/ipu3.cpp | 216 +++++++++++++++++++++------
> >>  1 file changed, 170 insertions(+), 46 deletions(-)
> >>
> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> index 164e187c769d..caf1051c58ab 100644
> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> @@ -171,17 +171,61 @@ private:
> >>  		CIO2Device cio2_;
> >>  		ImgUDevice *imgu_;
> >>
> >> -		Stream stream_;
> >> +		Stream outStream_;
> >> +		Stream vfStream_;
> >> +
> >> +		unsigned int activeStreamsMask;
> >>  	};
> >>
> >>  	static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> >>
> >> +	static constexpr unsigned int IPU3_STREAM_OUTPUT = BIT(0);
> >> +	static constexpr unsigned int IPU3_STREAM_VF = BIT(1);
> >> +
> >>  	IPU3CameraData *cameraData(const Camera *camera)
> >>  	{
> >>  		return static_cast<IPU3CameraData *>(
> >>  			PipelineHandler::cameraData(camera));
> >>  	}
> >>
> >> +	bool isOutput(IPU3CameraData *data, Stream *stream)
> >> +	{
> >> +		return &data->outStream_ == stream;
> >> +	}
> >
> > You're missing blank lines between functions.
> >
> 
> That was a style choice, to make those functions look more 'compact'
> together. But since both you and Niklas pointed this out, I'll change
> it..
> 
> >> +	bool isOutputActive(IPU3CameraData *data)
> >> +	{
> >> +		return (data->activeStreamsMask & IPU3_STREAM_OUTPUT) ?
> >> +			true : false;
> >> +	}
> >> +	void setOutputActive(IPU3CameraData *data)
> >> +	{
> >> +		data->activeStreamsMask |= IPU3_STREAM_OUTPUT;
> >> +	}
> >> +	bool isViewfinder(IPU3CameraData *data, Stream *stream)
> >> +	{
> >> +		return &data->vfStream_ == stream;
> >> +	}
> >> +	bool isViewfinderActive(IPU3CameraData *data)
> >> +	{
> >> +		return (data->activeStreamsMask & IPU3_STREAM_VF) ?
> >> +			true : false;
> >> +	}
> >> +	void setViewfinderActive(IPU3CameraData *data)
> >> +	{
> >> +		data->activeStreamsMask |= IPU3_STREAM_VF;
> >> +	}
> >> +	bool isStreamActive(IPU3CameraData *data, Stream *stream)
> >> +	{
> >> +		if (isOutput(data, stream) &&
> >> +		    isOutputActive(data))
> >> +			return true;
> >> +		if (isViewfinder(data, stream) &&
> >> +		    isViewfinderActive(data))
> >> +			return true;
> >
> > How about subclassing the Stream class and adding an active flag,
> > instead of storing that in a mask at the camera data level ? I think
> > you'll have more data to store in the IPU3CameraStream class.
> 
> By sub-classing Stream I can collect per-Stream informations, like an
> 'active' flag, what I need here is a global status that makes possible
> to do choices like "should I configure viewfinder now that I'm
> configuring output too, as viewfinder is not part of the active stream
> list?"

I agree that a global bitmask will also be needed, but you may be able
to turn it into local variables if you have a per-stream active state.

> True I can store a per-stream flag and check for that in each stream
> the pipeline handler supports. I'll see how many other things could
> fit in a Stream sub-class and consider this option, thanks!

Exactly :-)

> >> +
> >> +		return false;
> >> +	}
> >> +
> >>  	int registerCameras();
> >>
> >>  	ImgUDevice imgu0_;
> >> @@ -210,7 +254,7 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,
> >>  {
> >>  	std::map<Stream *, StreamConfiguration> configs;
> >>  	IPU3CameraData *data = cameraData(camera);
> >> -	StreamConfiguration *config = &configs[&data->stream_];
> >> +	StreamConfiguration config = {};
> >>
> >>  	/*
> >>  	 * FIXME: Soraka: the maximum resolution reported by both sensors
> >> @@ -220,52 +264,93 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,
> >>  	 *
> >>  	 * \todo Clarify ImgU alignement requirements.
> >>  	 */
> >> -	config->width = 2560;
> >> -	config->height = 1920;
> >> -	config->pixelFormat = V4L2_PIX_FMT_NV12;
> >> -	config->bufferCount = IPU3_BUFFER_COUNT;
> >> +	config.width = 2560;
> >> +	config.height = 1920;
> >> +	config.pixelFormat = V4L2_PIX_FMT_NV12;
> >> +	config.bufferCount = IPU3_BUFFER_COUNT;
> >> +
> >> +	configs[&data->outStream_] = config;
> >
> > A bit inefficient as you end up copying the data twice, but probably
> > not the end of the world.
> >
> >> +	LOG(IPU3, Debug)
> >> +		<< "Stream 'output' format set to " << config.width << "x"
> >> +		<< config.height << "-0x" << std::hex << std::setfill('0')
> >> +		<< std::setw(8) << config.pixelFormat;
> >
> > If you also stored the stream name in the IPU3CameraStream (or
> > IPU3Stream ?) class I think you could avoid code duplication.
> >
> >> +	configs[&data->vfStream_] = config;
> >
> > Can both streams scale ?
> 
> Can't they?

I don't know, it was a true question :-)

> Why is it relevant here?

Because you're configuring both streams with the same output size,
different than the sensor size, so you need the scale on both streams.

> >>  	LOG(IPU3, Debug)
> >> -		<< "Stream format set to " << config->width << "x"
> >> -		<< config->height << "-0x" << std::hex << std::setfill('0')
> >> -		<< std::setw(8) << config->pixelFormat;
> >> +		<< "Stream 'viewfinder' format set to " << config.width << "x"
> >> +		<< config.height << "-0x" << std::hex << std::setfill('0')
> >> +		<< std::setw(8) << config.pixelFormat;
> >>
> >>  	return configs;
> >>  }
> >>
> >>  int PipelineHandlerIPU3::configureStreams(Camera *camera,
> >> -					  std::map<Stream *, StreamConfiguration> &config)
> >> +					  std::map<Stream *,
> >> +						   StreamConfiguration> &config)
> >
> > That doesn't look nice, I think you could keep the longer line instead.
> 
> I like 80-cols :(

So do I, but something small exceptions are useful too :-)

> >>  {
> >>  	IPU3CameraData *data = cameraData(camera);
> >> -	const StreamConfiguration &cfg = config[&data->stream_];
> >> +	StreamConfiguration CIO2Config = {};
> >
> > Variables should start with lower case.
> 
> Ack.. cio2Config then
> 
> >>  	CIO2Device *cio2 = &data->cio2_;
> >>  	ImgUDevice *imgu = data->imgu_;
> >>  	int ret;
> >>
> >> -	LOG(IPU3, Info)
> >> -		<< "Requested image format " << cfg.width << "x"
> >> -		<< cfg.height << "-0x" << std::hex << std::setfill('0')
> >> -		<< std::setw(8) << cfg.pixelFormat << " on camera '"
> >> -		<< camera->name() << "'";
> >> +	/* Remove previously configured stream masks to store the new ones. */
> >> +	data->activeStreamsMask = 0;
> >> +	for (auto const &streamConfig : config) {
> >> +		Stream *stream = streamConfig.first;
> >> +		const StreamConfiguration &cfg = streamConfig.second;
> >>
> >> -	/*
> >> -	 * Verify that the requested size respects the IPU3 alignement
> >> -	 * requirements (the image width shall be a multiple of 8 pixels and
> >> -	 * its height a multiple of 4 pixels) and the camera maximum sizes.
> >> -	 *
> >> -	 * \todo: consider the BDS scaling factor requirements:
> >> -	 * "the downscaling factor must be an integer value multiple of 1/32"
> >> -	 */
> >> -	if (cfg.width % 8 || cfg.height % 4) {
> >> -		LOG(IPU3, Error) << "Invalid stream size: bad alignment";
> >> -		return -EINVAL;
> >> -	}
> >> +		/*
> >> +		 * Verify that the requested size respects the IPU3 alignement
> >> +		 * requirements (the image width shall be a multiple of 8
> >> +		 * pixels and its height a multiple of 4 pixels) and the camera
> >> +		 * maximum sizes.
> >> +		 *
> >> +		 * \todo: consider the BDS scaling factor requirements: "the
> >> +		 * downscaling factor must be an integer value multiple of
> >> +		 * 1/32"
> >
> > While at it, s/consider/Consider/ and s/32"/32"./
> >
> >> +		 */
> >> +		if (cfg.width % 8 || cfg.height % 4) {
> >> +			LOG(IPU3, Error)
> >> +				<< "Invalid stream size: bad alignment";
> >> +			return -EINVAL;
> >> +		}
> >>
> >> -	if (cfg.width > cio2->maxSize_.width ||
> >> -	    cfg.height > cio2->maxSize_.height) {
> >> -		LOG(IPU3, Error)
> >> -			<< "Invalid stream size: larger than sensor resolution";
> >> -		return -EINVAL;
> >> +		if (cfg.width > cio2->maxSize_.width ||
> >> +		    cfg.height > cio2->maxSize_.height) {
> >> +			LOG(IPU3, Error)
> >> +				<< "Invalid stream size: larger than sensor resolution";
> >> +			return -EINVAL;
> >> +		}
> >> +
> >> +		LOG(IPU3, Info)
> >> +			<< "Requested image format " << cfg.width << "x"
> >> +			<< cfg.height << "-0x" << std::hex << std::setw(8)
> >> +			<< cfg.pixelFormat << " on camera'"
> >> +			<< camera->name() << "'";
> >
> > That will be a bit confusing if you don't print the stream name, again,
> > with a Stream subclass, you could easily get the name :-)
> >
> >> +
> >> +		/*
> >> +		 * FIXME: As viewfinder should be operated even when
> >> +		 * applications do not intend to use it, we need to keep track
> >> +		 * of which streams have to be configured, to make meaningful
> >> +		 * decisions at configure and request queueing time.
> >> +		 *
> >> +		 * Walk here all the streams to configure and collect the
> >> +		 * active ones in a bitmaks.
> >> +		 */
> >> +		if (isOutput(data, stream))
> >> +			setOutputActive(data);
> >> +		if (isViewfinder(data, stream))
> >> +			setViewfinderActive(data);
> >> +
> >> +		/*
> >> +		 * Collect the maximum width and height: IPU3 can downscale
> >> +		 * only.
> >> +		 */
> >> +		if (cfg.width > CIO2Config.width)
> >> +			CIO2Config.width = cfg.width;
> >> +		if (cfg.height > CIO2Config.height)
> >> +			CIO2Config.height = cfg.height;
> >
> > Sounds good, but it's a bit strange to store that in a
> > StreamConfiguration, given that you only use it to configure the CIO2
> > and the ImgU input. How about replacing that by Size ?
> 
> That's also a possibility, I only use the width and height (I now
> wonder what should I do with the configuration's pixelformat fields though)
> 
> >>  	}
> >>
> >>  	/*
> >> @@ -281,26 +366,62 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
> >>  	 * adjusted format to be propagated to the ImgU output devices.
> >>  	 */
> >>  	V4L2DeviceFormat cio2Format = {};
> >> -	ret = cio2->configure(cfg, &cio2Format);
> >> +	ret = cio2->configure(CIO2Config, &cio2Format);
> >>  	if (ret)
> >>  		return ret;
> >>
> >> -	ret = imgu->configureInput(cfg, &cio2Format);
> >> +	ret = imgu->configureInput(CIO2Config, &cio2Format);
> >>  	if (ret)
> >>  		return ret;
> >>
> >>  	/* Apply the format to the ImgU output, viewfinder and stat. */
> >> -	ret = imgu->configureOutput(&imgu->output_, cfg);
> >> -	if (ret)
> >> -		return ret;
> >> +	for (auto const &streamConfig : config) {
> >> +		Stream *stream = streamConfig.first;
> >> +		const StreamConfiguration &cfg = streamConfig.second;
> >>
> >> -	ret = imgu->configureOutput(&imgu->viewfinder_, cfg);
> >> -	if (ret)
> >> -		return ret;
> >> +		if (isOutput(data, stream)) {
> >> +			ret = imgu->configureOutput(&imgu->output_, cfg);
> >> +			if (ret)
> >> +				return ret;
> >>
> >> -	ret = imgu->configureOutput(&imgu->stat_, cfg);
> >> -	if (ret)
> >> -		return ret;
> >> +			ret = imgu->configureOutput(&imgu->stat_, cfg);
> >> +			if (ret)
> >> +				return ret;
> >> +
> >> +
> >> +			if (isViewfinderActive(data))
> >> +				continue;
> >> +
> >> +			/*
> >> +			 * Configure viewfinder using the output stream
> >> +			 * configuration if it is not part of the active
> >> +			 * streams list.
> >> +			 */
> >> +			ret = imgu->configureOutput(&imgu->viewfinder_, cfg);
> >> +			if (ret)
> >> +				return ret;
> >> +		} else if (isViewfinder(data, stream)) {
> >> +			ret = imgu->configureOutput(&imgu->viewfinder_, cfg);
> >> +			if (ret)
> >> +				return ret;
> >> +
> >> +			if (isOutputActive(data))
> >> +				continue;
> >> +
> >> +			/*
> >> +			 * Configure output using the viewfinder stream
> >> +			 * configuration if it is not part of the active
> >> +			 * streams list.
> >> +			 */
> >> +			ret = imgu->configureOutput(&imgu->output_, cfg);
> >> +			if (ret)
> >> +				return ret;
> >> +
> >> +			ret = imgu->configureOutput(&imgu->stat_, cfg);
> >> +			if (ret)
> >> +				return ret;
> >> +		}
> >> +	}
> >
> > This looks quite complicated. I think you can configure the streams
> > included in the configuration in the loop without caring about their
> > type, and then configure the streams that are not included outside of
> > the loop. The code would look simpler.
> 
> Niklas had a similar idea, I'll try to see how it looks like.
> 
> >>  	return 0;
> >>  }
> >> @@ -404,7 +525,7 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
> >>  {
> >>  	IPU3CameraData *data = cameraData(camera);
> >>  	V4L2Device *output = data->imgu_->output_.dev;
> >> -	Stream *stream = &data->stream_;
> >> +	Stream *stream = &data->outStream_;
> >>
> >>  	/* Queue a buffer to the ImgU output for capture. */
> >>  	Buffer *buffer = request->findBuffer(stream);
> >> @@ -554,7 +675,10 @@ int PipelineHandlerIPU3::registerCameras()
> >>  	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_ };
> >> +		std::set<Stream *> streams = {
> >> +			&data->outStream_,
> >> +			&data->vfStream_,
> >> +		};
> >>  		CIO2Device *cio2 = &data->cio2_;
> >>
> >>  		ret = cio2->init(cio2MediaDev_.get(), id);

-- 
Regards,

Laurent Pinchart