[libcamera-devel] [PATCH v6 1/6] libcamera: ipu3: Create camera with 2 streams

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Apr 18, 2019 at 06:46:33PM +0200, Jacopo Mondi wrote:
> Sub-class the Stream class with an IPU3-specific implementation and
> 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/streams/

> 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, configure 'output', 'viewfinder'
> and 'stat' regardless of the user-requested active streams.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 182 +++++++++++++++++++--------
>  1 file changed, 130 insertions(+), 52 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 64b4210b4ce3..b9eb872fd5b2 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -62,7 +62,7 @@ public:
>  	}
>  
>  	int init(MediaDevice *media, unsigned int index);
> -	int configureInput(const StreamConfiguration &config,
> +	int configureInput(const Size &size,
>  			   V4L2DeviceFormat *inputFormat);
>  	int configureOutput(ImgUOutput *output,
>  			    const StreamConfiguration &config);
> @@ -112,7 +112,7 @@ public:
>  	}
>  
>  	int init(const MediaDevice *media, unsigned int index);
> -	int configure(const StreamConfiguration &config,
> +	int configure(const Size &size,
>  		      V4L2DeviceFormat *outputFormat);
>  
>  	BufferPool *exportBuffers();
> @@ -130,6 +130,19 @@ public:
>  	BufferPool pool_;
>  };
>  
> +class IPU3Stream : public Stream
> +{
> +public:
> +	IPU3Stream()
> +		: device_(nullptr), cfg_(nullptr)
> +	{
> +	}
> +
> +	std::string name_;
> +	ImgUDevice::ImgUOutput *device_;
> +	const StreamConfiguration *cfg_;
> +};
> +
>  class PipelineHandlerIPU3 : public PipelineHandler
>  {
>  public:
> @@ -170,7 +183,8 @@ private:
>  		CIO2Device cio2_;
>  		ImgUDevice *imgu_;
>  
> -		Stream stream_;
> +		IPU3Stream outStream_;
> +		IPU3Stream vfStream_;
>  	};
>  
>  	static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> @@ -209,7 +223,7 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,
>  {
>  	CameraConfiguration configs;
>  	IPU3CameraData *data = cameraData(camera);
> -	StreamConfiguration *config = &configs[&data->stream_];
> +	StreamConfiguration config = {};
>  
>  	/*
>  	 * FIXME: Soraka: the maximum resolution reported by both sensors
> @@ -219,15 +233,24 @@ 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 '" << data->outStream_.name_ << "' set to "
> +		<< config.width << "x" << config.height << "-0x"
> +		<< std::hex << std::setfill('0') << std::setw(8)
> +		<< config.pixelFormat;

If my recent patches get in first, you can use config.toString() here
and below.

>  
> +	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 '" << data->vfStream_.name_ << "' set to "
> +		<< config.width << "x" << config.height << "-0x"
> +		<< std::hex << std::setfill('0') << std::setw(8)
> +		<< config.pixelFormat;
>  
>  	return configs;
>  }
> @@ -236,30 +259,50 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
>  					  const CameraConfiguration &config)
>  {
>  	IPU3CameraData *data = cameraData(camera);
> -	const StreamConfiguration &cfg = config[&data->stream_];
> +	IPU3Stream *outStream = &data->outStream_;
> +	IPU3Stream *vfStream = &data->vfStream_;
>  	CIO2Device *cio2 = &data->cio2_;
>  	ImgUDevice *imgu = data->imgu_;
> +	Size sensorSize = {};
>  	int ret;
>  
> -	/*
> -	 * 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;
> -	}
> +	for (Stream *s : config) {
> +		IPU3Stream *stream = static_cast<IPU3Stream *>(s);
> +		const StreamConfiguration &cfg = config[stream];
>  
> -	const Size &resolution = cio2->sensor_->resolution();
> -	if (cfg.width > resolution.width ||
> -	    cfg.height > resolution.height) {
> -		LOG(IPU3, Error)
> -			<< "Invalid stream size: larger than sensor resolution";
> -		return -EINVAL;
> +		/*
> +		 * Verify that the requested size respects the IPU3 alignment
> +		 * 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;
> +		}
> +
> +		const Size &resolution = cio2->sensor_->resolution();
> +		if (cfg.width > resolution.width ||
> +		    cfg.height > resolution.height) {
> +			LOG(IPU3, Error)
> +				<< "Invalid stream size: larger than sensor resolution";
> +			return -EINVAL;
> +		}
> +
> +		/*
> +		 * Collect the maximum width and height: IPU3 can downscale
> +		 * only.
> +		 */
> +		if (cfg.width > sensorSize.width)
> +			sensorSize.width = cfg.width;
> +		if (cfg.height > sensorSize.height)
> +			sensorSize.height = cfg.height;
> +
> +		stream->cfg_ = &cfg;

You're storing a pointer to what may well be a temporary. As the pointer
is used in this function only I'd prefer storing it in local variables
instead. There's a too high chance we'll run into problems later
otherwise.

Apart from this the patch looks good to me.

>  	}
>  
>  	/*
> @@ -275,24 +318,51 @@ 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(sensorSize, &cio2Format);
>  	if (ret)
>  		return ret;
>  
> -	ret = imgu->configureInput(cfg, &cio2Format);
> +	ret = imgu->configureInput(sensorSize, &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;
> +	/* Apply the format to the configured streams output devices. */
> +	for (Stream *s : config) {
> +		IPU3Stream *stream = static_cast<IPU3Stream *>(s);
>  
> -	ret = imgu->configureOutput(&imgu->viewfinder_, cfg);
> -	if (ret)
> -		return ret;
> +		ret = imgu->configureOutput(stream->device_, *stream->cfg_);
> +		if (ret)
> +			return ret;
> +	}
>  
> -	ret = imgu->configureOutput(&imgu->stat_, cfg);
> +	/*
> +	 * As we need to set format also on the non-active streams, use
> +	 * the configuration of the active one for that purpose (there should
> +	 * be at least one active stream in the configuration request).
> +	 */
> +	if (!outStream->cfg_) {
> +		ret = imgu->configureOutput(outStream->device_,
> +					    *vfStream->cfg_);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (!vfStream->cfg_) {
> +		ret = imgu->configureOutput(vfStream->device_,
> +					    *outStream->cfg_);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/*
> +	 * Apply the largest available format to the stat node.
> +	 * \todo Revise this when we'll actually use the stat node.
> +	 */
> +	StreamConfiguration statConfig = {};
> +	statConfig.width = cio2Format.width;
> +	statConfig.height = cio2Format.height;
> +
> +	ret = imgu->configureOutput(&imgu->stat_, statConfig);
>  	if (ret)
>  		return ret;
>  
> @@ -407,7 +477,7 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
>  {
>  	IPU3CameraData *data = cameraData(camera);
>  	V4L2Device *output = data->imgu_->output_.dev;
> -	Stream *stream = &data->stream_;
> +	IPU3Stream *stream = &data->outStream_;
>  
>  	/* Queue a buffer to the ImgU output for capture. */
>  	Buffer *buffer = request->findBuffer(stream);
> @@ -558,7 +628,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);
> @@ -566,11 +639,16 @@ int PipelineHandlerIPU3::registerCameras()
>  			continue;
>  
>  		/**
> -		 * \todo Dynamically assign ImgU devices; as of now, limit
> -		 * support to two cameras only, and assign imgu0 to the first
> -		 * one and imgu1 to the second.
> +		 * \todo Dynamically assign ImgU and output devices to each
> +		 * stream and camera; as of now, limit support to two cameras
> +		 * only, and assign imgu0 to the first one and imgu1 to the
> +		 * second.
>  		 */
>  		data->imgu_ = numCameras ? &imgu1_ : &imgu0_;
> +		data->outStream_.device_ = &data->imgu_->output_;
> +		data->outStream_.name_ = "output";
> +		data->vfStream_.device_ = &data->imgu_->viewfinder_;
> +		data->vfStream_.name_ = "viewfinder";
>  
>  		/*
>  		 * Connect video devices' 'bufferReady' signals to their
> @@ -721,12 +799,12 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)
>  
>  /**
>   * \brief Configure the ImgU unit input
> - * \param[in] config The requested stream configuration
> + * \param[in] size The ImgU input frame size
>   * \param[in] inputFormat The format to be applied to ImgU input
>   *
>   * \return 0 on success or a negative error code otherwise
>   */
> -int ImgUDevice::configureInput(const StreamConfiguration &config,
> +int ImgUDevice::configureInput(const Size &size,
>  			       V4L2DeviceFormat *inputFormat)
>  {
>  	/* Configure the ImgU input video device with the requested sizes. */
> @@ -764,8 +842,8 @@ int ImgUDevice::configureInput(const StreamConfiguration &config,
>  			 << rect.toString();
>  
>  	V4L2SubdeviceFormat imguFormat = {};
> -	imguFormat.width = config.width;
> -	imguFormat.height = config.height;
> +	imguFormat.width = size.width;
> +	imguFormat.height = size.height;
>  	imguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;
>  
>  	ret = imgu_->setFormat(PAD_INPUT, &imguFormat);
> @@ -1080,12 +1158,12 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
>  
>  /**
>   * \brief Configure the CIO2 unit
> - * \param[in] config The requested configuration
> + * \param[in] size The requested CIO2 output frame size
>   * \param[out] outputFormat The CIO2 unit output image format
>   *
>   * \return 0 on success or a negative error code otherwise
>   */
> -int CIO2Device::configure(const StreamConfiguration &config,
> +int CIO2Device::configure(const Size &size,
>  			  V4L2DeviceFormat *outputFormat)
>  {
>  	V4L2SubdeviceFormat sensorFormat;
> @@ -1099,7 +1177,7 @@ int CIO2Device::configure(const StreamConfiguration &config,
>  					    MEDIA_BUS_FMT_SGBRG10_1X10,
>  					    MEDIA_BUS_FMT_SGRBG10_1X10,
>  					    MEDIA_BUS_FMT_SRGGB10_1X10 },
> -					  Size(config.width, config.height));
> +					  size);
>  	ret = sensor_->setFormat(&sensorFormat);
>  	if (ret)
>  		return ret;

-- 
Regards,

Laurent Pinchart