[libcamera-devel] [PATCH v7 4/8] libcamera: ipu3: Use roles in stream configuration

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Fri, Apr 19, 2019 at 12:18:35PM +0200, Jacopo Mondi wrote:
> Use and inspect the stream roles provided by the application to
> streamConfiguration() to assign streams to their intended roles and
> return a default configuration associated with them.
> 
> Support a limited number of usages, with the viewfinder stream able to
> capture both continuous video streams and still images, and the main
> output stream supporting still images images. This is an artificial

s/images images/images/

> limitation until we figure out the exact capabilities of the hardware.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 113 ++++++++++++++++++++-------
>  1 file changed, 83 insertions(+), 30 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 9b2d96f22b2b..45edc24b5d4c 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -5,6 +5,7 @@
>   * ipu3.cpp - Pipeline handler for Intel IPU3
>   */
>  
> +#include <algorithm>
>  #include <iomanip>
>  #include <memory>
>  #include <vector>
> @@ -222,38 +223,90 @@ CameraConfiguration
>  PipelineHandlerIPU3::streamConfiguration(Camera *camera,
>  					 const std::vector<StreamUsage> &usages)
>  {
> -	CameraConfiguration configs;
>  	IPU3CameraData *data = cameraData(camera);
> -	StreamConfiguration config = {};
> +	CameraConfiguration cameraConfig;
> +	std::set<IPU3Stream *> streams = {
> +		&data->outStream_,
> +		&data->vfStream_,
> +	};
>  
> -	/*
> -	 * FIXME: Soraka: the maximum resolution reported by both sensors
> -	 * (2592x1944 for ov5670 and 4224x3136 for ov13858) are returned as
> -	 * default configurations but they're not correctly processed by the
> -	 * ImgU. Resolutions up tp 2560x1920 have been validated.
> -	 *
> -	 * \todo Clarify ImgU alignement requirements.
> -	 */
> -	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;
> -
> -	configs[&data->vfStream_] = config;
> -	LOG(IPU3, Debug)
> -		<< "Stream '" << data->vfStream_.name_ << "' set to "
> -		<< config.width << "x" << config.height << "-0x"
> -		<< std::hex << std::setfill('0') << std::setw(8)
> -		<< config.pixelFormat;
> -
> -	return configs;
> +	for (const StreamUsage &usage : usages) {
> +		std::vector<IPU3Stream *>::iterator found;

This variable isn't used anymore. No warning from the compiler ?

> +		StreamUsage::Role role = usage.role();
> +		StreamConfiguration streamConfig = {};
> +		IPU3Stream *stream = nullptr;
> +
> +		if (role == StreamUsage::Role::StillCapture) {
> +			/*
> +			 * Don't allow viewfinder or video capture on the
> +			 * 'output stream. This is an artificial limitation
> +			 * until we figure out the capabilities of the
> +			 * hardware.
> +			 */
> +			if (streams.find(&data->outStream_) != streams.end())
> +				stream = &data->outStream_;
> +			else if (streams.find(&data->vfStream_) != streams.end())
> +				stream = &data->vfStream_;
> +			else
> +				goto error;
> +
> +			/*
> +			 * FIXME: Soraka: the maximum resolution reported by
> +			 * both sensors (2592x1944 for ov5670 and 4224x3136 for
> +			 * ov13858) are returned as default configurations but
> +			 * they're not correctly processed by the ImgU.
> +			 * Resolutions up tp 2560x1920 have been validated.
> +			 *
> +			 * \todo Clarify ImgU alignment requirements.
> +			 */
> +			streamConfig.width = 2560;
> +			streamConfig.height = 1920;
> +		} else if (role == StreamUsage::Role::Viewfinder ||
> +			   role == StreamUsage::Role::VideoRecording) {
> +			/*
> +			 * We can't use the 'output' stream for viewfinder or
> +			 * video capture usages.
> +			 */

I'd duplicate here the information from the commit message explaining
that the limitation is artificial, maybe with a \todo

> +			if (streams.find(&data->vfStream_) == streams.end())
> +				goto error;
> +
> +			stream = &data->vfStream_;
> +
> +

Extra blank line.

> +			/*
> +			 * Align the requested viewfinder size to the
> +			 * maximum available sensor resolution and to the
> +			 * IPU3 alignment constraints.
> +			 */
> +			const Size &res = data->cio2_.sensor_->resolution();
> +			unsigned int width = std::min(usage.size().width,
> +						      res.width);
> +			unsigned int height = std::min(usage.size().height,
> +						       res.height);

Should we set a minimum size too ?

We need to figure out a policy for the streamsConfiguration() operation.
It doesn't have to be solved as part of this series, but we need to
document it.

> +			streamConfig.width = width & ~7;
> +			streamConfig.height = height & ~3;
> +		}
> +
> +		streams.erase(stream);
> +
> +		streamConfig.pixelFormat = V4L2_PIX_FMT_NV12;
> +		streamConfig.bufferCount = IPU3_BUFFER_COUNT;
> +
> +		cameraConfig[stream] = streamConfig;
> +
> +		LOG(IPU3, Debug)
> +			<< "Stream '" << stream->name_ << "' format set to "
> +			<< streamConfig.width << "x" << streamConfig.height
> +			<< "-0x" << std::hex << std::setfill('0')
> +			<< std::setw(8) << streamConfig.pixelFormat;
> +	}
> +
> +	return cameraConfig;
> +
> +error:
> +	LOG(IPU3, Error) << "Requested stream roles not supported";
> +
> +	return CameraConfiguration{};
>  }
>  
>  int PipelineHandlerIPU3::configureStreams(Camera *camera,

-- 
Regards,

Laurent Pinchart