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

907 diff mbox series

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@jmondi.org>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 216 +++++++++++++++++++++------
 1 file changed, 170 insertions(+), 46 deletions(-)

Hi Jacopo,

Thanks for your patch.

On 2019-04-03 17:07:29 +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@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;
> +	}
> +	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;
> +
> +		return false;
> +	}
> +

Nit picking, I find the lack of new lines between functions above hard 
to read :-) Also maybe you should add this new bit field and the helper 
functions in a separate patch.

>  	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;
> +	LOG(IPU3, Debug)
> +		<< "Stream 'output' format set to " << config.width << "x"
> +		<< config.height << "-0x" << std::hex << std::setfill('0')
> +		<< std::setw(8) << config.pixelFormat;
>  
> +	configs[&data->vfStream_] = config;
>  	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;

This is not correct as streamConfiguration() would always return a map 
with 2 elements even if only one stream is requested. Looking at your 
private branch the top commit which make use of the stream usages 
(stream roles in your case is it's based on the RFC) solves this in a 
correct way. Maybe you should fold that change in here and depend on the 
stream usage serries?

>  
>  	return configs;
>  }
>  
>  int PipelineHandlerIPU3::configureStreams(Camera *camera,
> -					  std::map<Stream *, StreamConfiguration> &config)
> +					  std::map<Stream *,
> +						   StreamConfiguration> &config)
>  {
>  	IPU3CameraData *data = cameraData(camera);
> -	const StreamConfiguration &cfg = config[&data->stream_];
> +	StreamConfiguration CIO2Config = {};
>  	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"
> +		 */
> +		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() << "'";
> +
> +		/*
> +		 * 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;
>  	}
>  
>  	/*
> @@ -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;
> +		}
> +	}

I see the need for the 'if (is*Active(data)) continue' logic but I 
winder if you could not be a bit more clever about it :-)

How about in the for (auto const &streamConfig : config) loop only 
creating a cfg_output and cfg_viewfinder and outside the loop call 
imgu->configureOutpu() once for each stream?

>  
>  	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);
> -- 
> 2.21.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

Thank you for the patch.

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@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.

> +	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.

> +
> +		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 ?

>  	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.

>  {
>  	IPU3CameraData *data = cameraData(camera);
> -	const StreamConfiguration &cfg = config[&data->stream_];
> +	StreamConfiguration CIO2Config = {};

Variables should start with lower case.

>  	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 ?

>  	}
>  
>  	/*
> @@ -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.

>  
>  	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);

Hi (again) Jacopo,

On 2019-04-05 13:22:47 +0200, Niklas Söderlund wrote:
> Hi Jacopo,
> 
> Thanks for your patch.
> 
> On 2019-04-03 17:07:29 +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@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;
> > +	}
> > +	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;
> > +
> > +		return false;
> > +	}
> > +
> 
> Nit picking, I find the lack of new lines between functions above hard 
> to read :-) Also maybe you should add this new bit field and the helper 
> functions in a separate patch.
> 
> >  	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;
> > +	LOG(IPU3, Debug)
> > +		<< "Stream 'output' format set to " << config.width << "x"
> > +		<< config.height << "-0x" << std::hex << std::setfill('0')
> > +		<< std::setw(8) << config.pixelFormat;
> >  
> > +	configs[&data->vfStream_] = config;
> >  	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;
> 
> This is not correct as streamConfiguration() would always return a map 
> with 2 elements even if only one stream is requested. Looking at your 
> private branch the top commit which make use of the stream usages 
> (stream roles in your case is it's based on the RFC) solves this in a 
> correct way. Maybe you should fold that change in here and depend on the 
> stream usage serries?

I just took the for mentioned patch of your for a test drive. Good news 
it work with small changes to incorporate the latest CameraConfiguration 
changes, really nice work extending the IPU3 pipeline handler to support 
multiple streams!

While doing so I noticed something you might want to consider when 
folding it into this series. The top patch in your private branch do not 
allow for multiple streams to have the same usage role, it's locked to 
one still image and one viewfinder.

    $ cam --camera "ov5670 4-0036 1" --capture --file \
        --stream role=still,width=2560,height=1920 \
        --stream role=viewfinder,width=640,height=480

Works as expected and frames form both streams are written to disc.

    $ cam --camera "ov5670 4-0036 1" --capture --file \
        --stream role=still,width=2560,height=1920 \
        --stream role=still,width=640,height=480

Do not work as the IPU3 pipeline handler do not allow for two still 
image streams. And the video usage role is not supported at all.

In my mental model of how this should work is that any combinations of 
stream usage roles should be supported by all pipeline handlers. The 
stream usage roles are used as 'hints' to the pipeline handlers to allow 
it to make better selection of which of its hardware pipelines to assign 
to each stream.

Maybe my mental model is wrong and your interpretation is the correct 
one. Maybe you share my model but as this patch is prototype as it's not 
yet posted to the ML. In any case I thought it best to bring it up so we 
can align our views and avoid unnecessary work :-)

> 
> >  
> >  	return configs;
> >  }
> >  
> >  int PipelineHandlerIPU3::configureStreams(Camera *camera,
> > -					  std::map<Stream *, StreamConfiguration> &config)
> > +					  std::map<Stream *,
> > +						   StreamConfiguration> &config)
> >  {
> >  	IPU3CameraData *data = cameraData(camera);
> > -	const StreamConfiguration &cfg = config[&data->stream_];
> > +	StreamConfiguration CIO2Config = {};
> >  	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"
> > +		 */
> > +		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() << "'";
> > +
> > +		/*
> > +		 * 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;
> >  	}
> >  
> >  	/*
> > @@ -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;
> > +		}
> > +	}
> 
> I see the need for the 'if (is*Active(data)) continue' logic but I 
> winder if you could not be a bit more clever about it :-)
> 
> How about in the for (auto const &streamConfig : config) loop only 
> creating a cfg_output and cfg_viewfinder and outside the loop call 
> imgu->configureOutpu() once for each stream?
> 
> >  
> >  	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);
> > -- 
> > 2.21.0
> > 
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
> 
> -- 
> Regards,
> Niklas Söderlund

Hi Niklas,

On Sat, Apr 06, 2019 at 01:40:22AM +0200, Niklas Söderlund wrote:
> On 2019-04-05 13:22:47 +0200, Niklas Söderlund wrote:
> > On 2019-04-03 17:07:29 +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@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;
> >> +	}
> >> +	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;
> >> +
> >> +		return false;
> >> +	}
> >> +
> > 
> > Nit picking, I find the lack of new lines between functions above hard 
> > to read :-) Also maybe you should add this new bit field and the helper 
> > functions in a separate patch.
> > 
> >>  	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;
> >> +	LOG(IPU3, Debug)
> >> +		<< "Stream 'output' format set to " << config.width << "x"
> >> +		<< config.height << "-0x" << std::hex << std::setfill('0')
> >> +		<< std::setw(8) << config.pixelFormat;
> >>  
> >> +	configs[&data->vfStream_] = config;
> >>  	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;
> > 
> > This is not correct as streamConfiguration() would always return a map 
> > with 2 elements even if only one stream is requested. Looking at your 
> > private branch the top commit which make use of the stream usages 
> > (stream roles in your case is it's based on the RFC) solves this in a 
> > correct way. Maybe you should fold that change in here and depend on the 
> > stream usage serries?
> 
> I just took the for mentioned patch of your for a test drive. Good news 
> it work with small changes to incorporate the latest CameraConfiguration 
> changes, really nice work extending the IPU3 pipeline handler to support 
> multiple streams!
> 
> While doing so I noticed something you might want to consider when 
> folding it into this series. The top patch in your private branch do not 
> allow for multiple streams to have the same usage role, it's locked to 
> one still image and one viewfinder.
> 
>     $ cam --camera "ov5670 4-0036 1" --capture --file \
>         --stream role=still,width=2560,height=1920 \
>         --stream role=viewfinder,width=640,height=480
> 
> Works as expected and frames form both streams are written to disc.
> 
>     $ cam --camera "ov5670 4-0036 1" --capture --file \
>         --stream role=still,width=2560,height=1920 \
>         --stream role=still,width=640,height=480
> 
> Do not work as the IPU3 pipeline handler do not allow for two still 
> image streams. And the video usage role is not supported at all.
> 
> In my mental model of how this should work is that any combinations of 
> stream usage roles should be supported by all pipeline handlers. The 
> stream usage roles are used as 'hints' to the pipeline handlers to allow 
> it to make better selection of which of its hardware pipelines to assign 
> to each stream.

I agree overall, but "any combination" may be a bit too much. Pipeline
handlers should all combinations that are achievable from a hardware
point of view. For instance, if an application requests two viewfinder
streams, or one viewfinder and one video recording stream, while the
hardware can produce two streams but only one of them can produce video,
while the other is limited to still images with a max frame rate of one
frame every 3 seconds, that request should likely fail.

> Maybe my mental model is wrong and your interpretation is the correct 
> one. Maybe you share my model but as this patch is prototype as it's not 
> yet posted to the ML. In any case I thought it best to bring it up so we 
> can align our views and avoid unnecessary work :-)
> 
> > 
> >>  
> >>  	return configs;
> >>  }
> >>  
> >>  int PipelineHandlerIPU3::configureStreams(Camera *camera,
> >> -					  std::map<Stream *, StreamConfiguration> &config)
> >> +					  std::map<Stream *,
> >> +						   StreamConfiguration> &config)
> >>  {
> >>  	IPU3CameraData *data = cameraData(camera);
> >> -	const StreamConfiguration &cfg = config[&data->stream_];
> >> +	StreamConfiguration CIO2Config = {};
> >>  	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"
> >> +		 */
> >> +		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() << "'";
> >> +
> >> +		/*
> >> +		 * 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;
> >>  	}
> >>  
> >>  	/*
> >> @@ -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;
> >> +		}
> >> +	}
> > 
> > I see the need for the 'if (is*Active(data)) continue' logic but I 
> > winder if you could not be a bit more clever about it :-)
> > 
> > How about in the for (auto const &streamConfig : config) loop only 
> > creating a cfg_output and cfg_viewfinder and outside the loop call 
> > imgu->configureOutpu() once for each stream?
> > 
> >>  
> >>  	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);

Hi Laurent,
 thanks for review,

On Fri, Apr 05, 2019 at 06:44:05PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> 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@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?"

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!

> > +
> > +		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? Why is it relevant here?

> >  	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 :(

> >  {
> >  	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.

Thanks
   j
> >
> >  	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

Hi Niklas,

On Fri, Apr 05, 2019 at 01:22:47PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your patch.
>
> On 2019-04-03 17:07:29 +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@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;
> > +	}
> > +	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;
> > +
> > +		return false;
> > +	}
> > +
>
> Nit picking, I find the lack of new lines between functions above hard
> to read :-) Also maybe you should add this new bit field and the helper
> functions in a separate patch.
>

Laurent had the same comment, I'll break them.

On the separate patch, as you seem to ask it quite often, I introduced
them here as they are used here... Introducing them separately without
users makes it hard for reviewer to see how they are actually used
(well, here it is quite trivial :)

I understand smaller patches are less tiring to review, but sometimes
demand the reviewer to jump from one patch to the other, which I
really hate, personally :)

I guess it's a matter of tastes...

Thanks
  j

> >  	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;
> > +	LOG(IPU3, Debug)
> > +		<< "Stream 'output' format set to " << config.width << "x"
> > +		<< config.height << "-0x" << std::hex << std::setfill('0')
> > +		<< std::setw(8) << config.pixelFormat;
> >
> > +	configs[&data->vfStream_] = config;
> >  	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;
>
> This is not correct as streamConfiguration() would always return a map
> with 2 elements even if only one stream is requested. Looking at your
> private branch the top commit which make use of the stream usages
> (stream roles in your case is it's based on the RFC) solves this in a
> correct way. Maybe you should fold that change in here and depend on the
> stream usage serries?
>
> >
> >  	return configs;
> >  }
> >
> >  int PipelineHandlerIPU3::configureStreams(Camera *camera,
> > -					  std::map<Stream *, StreamConfiguration> &config)
> > +					  std::map<Stream *,
> > +						   StreamConfiguration> &config)
> >  {
> >  	IPU3CameraData *data = cameraData(camera);
> > -	const StreamConfiguration &cfg = config[&data->stream_];
> > +	StreamConfiguration CIO2Config = {};
> >  	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"
> > +		 */
> > +		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() << "'";
> > +
> > +		/*
> > +		 * 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;
> >  	}
> >
> >  	/*
> > @@ -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;
> > +		}
> > +	}
>
> I see the need for the 'if (is*Active(data)) continue' logic but I
> winder if you could not be a bit more clever about it :-)
>
> How about in the for (auto const &streamConfig : config) loop only
> creating a cfg_output and cfg_viewfinder and outside the loop call
> imgu->configureOutpu() once for each stream?
>
> >
> >  	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);
> > --
> > 2.21.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund

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@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);

Hi Laurent,

On Mon, Apr 08, 2019 at 04:02:56PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you.
>

[snip]

> > >> +	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.
>

Oh, now I get what you mean here... Yes, both streams can scale
indeed. According to upstream IPU3 developers viewfinder could
additionally perform composition, but the field of view selection
procedure is handled internally by the driver, as their API
implementation currently does not allow to apply a composition
rectangle to the viewfinder's pad)

Thanks
  j

> > >>  	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