[libcamera-devel] [PATCH v4 18/31] libcamera: ipu3: Create camera with 2 streams

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Mar 20, 2019 at 05:30:42PM +0100, Jacopo Mondi wrote:
> Create each IPU3 camera with two streams: 'output' and 'viewfinder'
> which represents the video stream from main and secondary ImgU output

s/represents/represent/
s/stream from main/streams for the main/

> respectively.
> 
> Return a default configuration for both streams in
> streamConfiguration().
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 42 ++++++++++++++++++----------
>  1 file changed, 28 insertions(+), 14 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 43fa0e4a7f9d..44e5eb48549e 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -120,6 +120,10 @@ private:
>  	static constexpr unsigned int IPU3_CIO2_BUFFER_COUNT = 4;
>  	static constexpr unsigned int IPU3_IMGU_BUFFER_COUNT = 4;
>  
> +	static constexpr unsigned int IPU3_STREAMS_COUNT = 2;
> +	static constexpr unsigned int IPU3_STREAM_OUTPUT = 0;
> +	static constexpr unsigned int IPU3_STREAM_VF = 1;
> +
>  	class IPU3CameraData : public CameraData
>  	{
>  	public:
> @@ -135,7 +139,7 @@ private:
>  		CIO2Device cio2;
>  		ImgUDevice *imgu;
>  
> -		Stream stream_;
> +		Stream streams_[IPU3_STREAMS_COUNT];

Unless you need to iterate over the streams array, it may be more
readable to use two variables (outputStream_ and viewfinderStream_ for
instance, or outStream_ and vfStream_ possibly).

>  	};
>  
>  	IPU3CameraData *cameraData(const Camera *camera)
> @@ -194,7 +198,7 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,
>  	std::map<Stream *, StreamConfiguration> configs;
>  	IPU3CameraData *data = cameraData(camera);
>  	V4L2Subdevice *sensor = data->cio2.sensor;
> -	StreamConfiguration *config = &configs[&data->stream_];
> +	StreamConfiguration config = {};
>  	unsigned int bestSize = 0;
>  
>  	const FormatEnum formats = sensor->formats(0);
> @@ -213,8 +217,8 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,
>  
>  			bestSize = frameSize;
>  
> -			config->width = range.maxWidth;
> -			config->height = range.maxHeight;
> +			config.width = range.maxWidth;
> +			config.height = range.maxHeight;
>  		}
>  	}
>  
> @@ -226,15 +230,22 @@ 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->streams_[IPU3_STREAM_OUTPUT]] = 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->streams_[IPU3_STREAM_VF]] = 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;
>  
>  	return configs;
>  }
> @@ -243,7 +254,7 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
>  					  std::map<Stream *, StreamConfiguration> &config)
>  {
>  	IPU3CameraData *data = cameraData(camera);
> -	const StreamConfiguration &cfg = config[&data->stream_];
> +	const StreamConfiguration &cfg = config[&data->streams_[0]];

This patch is difficult to review, it only contains a partial
implementation. The code seems to go in the right direction, but in
general the series isn't bisectable. I'm not sure a squashed patch would
be more difficult to review...

>  	V4L2Device *viewfinder = data->imgu->viewfinder;
>  	V4L2Device *output = data->imgu->output;
>  	V4L2Device *input = data->imgu->input;
> @@ -506,7 +517,7 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
>  	V4L2Device *viewfinder = data->imgu->viewfinder;
>  	V4L2Device *output = data->imgu->output;
>  	V4L2Device *stat = data->imgu->stat;
> -	Stream *stream = &data->stream_;
> +	Stream *stream = &data->streams_[0];
>  	Buffer *tmpBuffer;
>  
>  	/*
> @@ -1097,7 +1108,10 @@ void 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->streams_[IPU3_STREAM_OUTPUT],
> +			&data->streams_[IPU3_STREAM_VF],
> +		};
>  		CIO2Device *cio2 = &data->cio2;
>  		int ret;
>  

-- 
Regards,

Laurent Pinchart