[libcamera-devel,PATCH/RFC,07/12] libcamera: camera: Add a validation API to the CameraConfiguration class

1224 diff mbox series

The CameraConfiguration class implements a simple storage of
StreamConfiguration with internal validation limited to verifying that
the stream configurations are not empty. Extend this mechanism by
implementing a smart validate() method backed by pipeline handlers.

This new mechanism changes the semantics of the camera configuration.
The Camera::generateConfiguration() operation still generates a default
configuration based on roles, but now also supports generating empty
configurations to be filled by applications. Applications can inspect
the configuration, optionally modify it, and validate it. The validation
implements "try" semantics and adjusts invalid configurations instead of
rejecting them completely. Applications then decide whether to accept
the modified configuration, or try again with a different set of
parameters. Once the configuration is valid, it is passed to
Camera::configure(), and pipeline handlers are guaranteed that the
configuration they receive is valid.

A reference to the Camera may need to be stored in the
CameraConfiguration derived classes in order to access it from their
validate() implementation. This must be stored as a std::shared_ptr<> as
the CameraConfiguration instances belong to applications. In order to
make this possible, make the Camera class inherit from
std::shared_from_this<>.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/camera.h               |  16 +-
 src/cam/main.cpp                         |   2 +-
 src/libcamera/camera.cpp                 |  90 +++++---
 src/libcamera/pipeline/ipu3/ipu3.cpp     | 255 ++++++++++++++++++-----
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 149 ++++++++++---
 src/libcamera/pipeline/uvcvideo.cpp      |  53 ++++-
 src/libcamera/pipeline/vimc.cpp          |  67 +++++-
 src/libcamera/pipeline_handler.cpp       |  17 +-
 test/camera/capture.cpp                  |   7 +-
 test/camera/configuration_default.cpp    |   4 +-
 test/camera/configuration_set.cpp        |   7 +-
 11 files changed, 521 insertions(+), 146 deletions(-)

Hi Laurent,

Thanks for your work.

On 2019-05-18 02:06:16 +0300, Laurent Pinchart wrote:
> The CameraConfiguration class implements a simple storage of
> StreamConfiguration with internal validation limited to verifying that
> the stream configurations are not empty. Extend this mechanism by
> implementing a smart validate() method backed by pipeline handlers.
> 
> This new mechanism changes the semantics of the camera configuration.
> The Camera::generateConfiguration() operation still generates a default
> configuration based on roles, but now also supports generating empty
> configurations to be filled by applications. Applications can inspect
> the configuration, optionally modify it, and validate it. The validation
> implements "try" semantics and adjusts invalid configurations instead of
> rejecting them completely. Applications then decide whether to accept
> the modified configuration, or try again with a different set of
> parameters. Once the configuration is valid, it is passed to
> Camera::configure(), and pipeline handlers are guaranteed that the
> configuration they receive is valid.
> 
> A reference to the Camera may need to be stored in the
> CameraConfiguration derived classes in order to access it from their
> validate() implementation. This must be stored as a std::shared_ptr<> as
> the CameraConfiguration instances belong to applications. In order to
> make this possible, make the Camera class inherit from
> std::shared_from_this<>.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/camera.h               |  16 +-
>  src/cam/main.cpp                         |   2 +-
>  src/libcamera/camera.cpp                 |  90 +++++---
>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 255 ++++++++++++++++++-----
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 149 ++++++++++---
>  src/libcamera/pipeline/uvcvideo.cpp      |  53 ++++-
>  src/libcamera/pipeline/vimc.cpp          |  67 +++++-
>  src/libcamera/pipeline_handler.cpp       |  17 +-
>  test/camera/capture.cpp                  |   7 +-
>  test/camera/configuration_default.cpp    |   4 +-
>  test/camera/configuration_set.cpp        |   7 +-
>  11 files changed, 521 insertions(+), 146 deletions(-)
> 
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 841e8fc505b9..276bf1fb1887 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -25,30 +25,38 @@ class Request;
>  class CameraConfiguration
>  {
>  public:
> +	enum Status {
> +		Valid,
> +		Adjusted,
> +		Invalid,
> +	};
> +
>  	using iterator = std::vector<StreamConfiguration>::iterator;
>  	using const_iterator = std::vector<StreamConfiguration>::const_iterator;
>  
> -	CameraConfiguration();
> +	virtual ~CameraConfiguration();
>  
>  	void addConfiguration(const StreamConfiguration &cfg);
> +	virtual Status validate() = 0;
>  
>  	iterator begin();
>  	iterator end();
>  	const_iterator begin() const;
>  	const_iterator end() const;
>  
> -	bool isValid() const;
>  	bool isEmpty() const;
>  	std::size_t size() const;
>  
>  	StreamConfiguration &operator[](unsigned int index);
>  	const StreamConfiguration &operator[](unsigned int index) const;
>  
> -private:
> +protected:
> +	CameraConfiguration();
> +
>  	std::vector<StreamConfiguration> config_;
>  };
>  
> -class Camera final
> +class Camera final : public std::enable_shared_from_this<Camera>
>  {
>  public:
>  	static std::shared_ptr<Camera> create(PipelineHandler *pipe,
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index 7a1b332f68c7..a962f94c8f59 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -116,7 +116,7 @@ static CameraConfiguration *prepareCameraConfig()
>  	}
>  
>  	CameraConfiguration *config = camera->generateConfiguration(roles);
> -	if (!config || !config->isValid()) {
> +	if (!config || config->size() != roles.size()) {
>  		std::cerr << "Failed to get default stream configuration"
>  			  << std::endl;
>  		delete config;
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 115cdb1c024b..572939956a34 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -52,6 +52,28 @@ LOG_DECLARE_CATEGORY(Camera)
>   * operator[](int) returns a reference to the StreamConfiguration based on its
>   * insertion index. Accessing a stream configuration with an invalid index
>   * results in undefined behaviour.
> + *
> + * CameraConfiguration instances are retrieved from the camera with
> + * Camera::generateConfiguration(). Applications may then inspect the
> + * configuration, modify it, and possibly add new stream configuration entries
> + * with addConfiguration(). Once the camera configuration satisfies the
> + * application, it shall be validated by a call to validate(). The validation
> + * implements "try" semantics: it adjusts invalid configurations to the closest
> + * achievable parameters instead of rejecting them completely. Applications
> + * then decide whether to accept the modified configuration, or try again with
> + * a different set of parameters. Once the configuration is valid, it is passed
> + * to Camera::configure().
> + */
> +
> +/**
> + * \enum CameraConfiguration::Status
> + * \brief Validity of a camera configuration
> + * \var CameraConfiguration::Valid
> + * The configuration is fully valid
> + * \var CameraConfiguration::Adjusted
> + * The configuration has been adjusted to a valid configuration
> + * \var CameraConfiguration::Invalid
> + * The configuration is invalid and can't be adjusted automatically
>   */
>  
>  /**
> @@ -73,6 +95,10 @@ CameraConfiguration::CameraConfiguration()
>  {
>  }
>  
> +CameraConfiguration::~CameraConfiguration()
> +{
> +}
> +
>  /**
>   * \brief Add a stream configuration to the camera configuration
>   * \param[in] cfg The stream configuration
> @@ -82,6 +108,33 @@ void CameraConfiguration::addConfiguration(const StreamConfiguration &cfg)
>  	config_.push_back(cfg);
>  }
>  
> +/**
> + * \fn CameraConfiguration::validate()
> + * \brief Validate and possibly adjust the camera configuration
> + *
> + * This method adjusts the camera configuration to the closest valid
> + * configuration and returns the validation status.
> + *
> + * \todo: Define exactly when to return each status code. Should stream
> + * parameters set to 0 by the caller be adjusted without returning Adjusted ?
> + * This would potentially be useful for applications but would get in the way
> + * in Camera::configure(). Do we need an extra status code to signal this ?
> + *
> + * \todo: Handle validation of buffers count when refactoring the buffers API.
> + *
> + * \return A CameraConfiguration::Status value that describes the validation
> + * status.
> + * \retval CameraConfiguration::Invalid The configuration is invalid and can't
> + * be adjusted. This may only occur in extreme cases such as when the
> + * configuration is empty.
> + * \retval CameraConfigutation::Adjusted The configuration has been adjusted
> + * and is now valid. Parameters may have changed for any stream, and stream
> + * configurations may have been removed. The caller shall check the
> + * configuration carefully.
> + * \retval CameraConfiguration::Valid The configuration was already valid and
> + * hasn't been adjusted.
> + */
> +
>  /**
>   * \brief Retrieve an iterator to the first stream configuration in the
>   * sequence
> @@ -123,29 +176,6 @@ CameraConfiguration::const_iterator CameraConfiguration::end() const
>  	return config_.end();
>  }
>  
> -/**
> - * \brief Check if the camera configuration is valid
> - *
> - * A camera configuration is deemed to be valid if it contains at least one
> - * stream configuration and all stream configurations contain valid information.
> - * Stream configurations are deemed to be valid if all fields are none zero.
> - *
> - * \return True if the configuration is valid
> - */
> -bool CameraConfiguration::isValid() const
> -{
> -	if (isEmpty())
> -		return false;
> -
> -	for (const StreamConfiguration &cfg : config_) {
> -		if (cfg.size.width == 0 || cfg.size.height == 0 ||
> -		    cfg.pixelFormat == 0 || cfg.bufferCount == 0)
> -			return false;
> -	}
> -
> -	return true;
> -}
> -
>  /**
>   * \brief Check if the camera configuration is empty
>   * \return True if the configuration is empty
> @@ -194,6 +224,11 @@ const StreamConfiguration &CameraConfiguration::operator[](unsigned int index) c
>  	return config_[index];
>  }
>  
> +/**
> + * \var CameraConfiguration::config_
> + * \brief The vector of stream configurations
> + */

I think this should go into an earlier patch in this series, right?

> +
>  /**
>   * \class Camera
>   * \brief Camera device
> @@ -551,10 +586,9 @@ CameraConfiguration *Camera::generateConfiguration(const StreamRoles &roles)
>   * The caller specifies which streams are to be involved and their configuration
>   * by populating \a config.
>   *
> - * The easiest way to populate the array of config is to fetch an initial
> - * configuration from the camera with generateConfiguration() and then change
> - * the parameters to fit the caller's need and once all the streams parameters
> - * are configured hand that over to configure() to actually setup the camera.
> + * The configuration is created by generateConfiguration(), and adjusted by the
> + * caller with CameraConfiguration::validate(). This method only accepts fully
> + * valid configurations and returns an error if \a config is not valid.
>   *
>   * Exclusive access to the camera shall be ensured by a call to acquire() prior
>   * to calling this function, otherwise an -EACCES error will be returned.
> @@ -579,7 +613,7 @@ int Camera::configure(CameraConfiguration *config)
>  	if (!stateBetween(CameraAcquired, CameraConfigured))
>  		return -EACCES;
>  
> -	if (!config->isValid()) {
> +	if (config->validate() != CameraConfiguration::Valid) {
>  		LOG(Camera, Error)
>  			<< "Can't configure camera with invalid configuration";
>  		return -EINVAL;
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 8430e0591a41..a5e71832d3c2 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -164,6 +164,33 @@ public:
>  	IPU3Stream vfStream_;
>  };
>  
> +class IPU3CameraConfiguration : public CameraConfiguration
> +{
> +public:
> +	IPU3CameraConfiguration(Camera *camera, IPU3CameraData *data);
> +
> +	Status validate() override;
> +
> +	const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
> +	const std::vector<const IPU3Stream *> &streams() { return streams_; }
> +
> +private:
> +	static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> +
> +	void adjustStream(unsigned int index, bool scale);
> +
> +	/*
> +	 * The IPU3CameraData instance is guaranteed to be valid as long as the
> +	 * corresponding Camera instance is valid. In order to borrow a
> +	 * reference to the camera data, store a new reference to the camera.
> +	 */
> +	std::shared_ptr<Camera> camera_;
> +	const IPU3CameraData *data_;
> +
> +	V4L2SubdeviceFormat sensorFormat_;
> +	std::vector<const IPU3Stream *> streams_;
> +};
> +
>  class PipelineHandlerIPU3 : public PipelineHandler
>  {
>  public:
> @@ -186,8 +213,6 @@ public:
>  	bool match(DeviceEnumerator *enumerator) override;
>  
>  private:
> -	static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> -
>  	IPU3CameraData *cameraData(const Camera *camera)
>  	{
>  		return static_cast<IPU3CameraData *>(
> @@ -202,6 +227,153 @@ private:
>  	MediaDevice *imguMediaDev_;
>  };
>  
> +IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera,
> +						 IPU3CameraData *data)
> +	: CameraConfiguration()
> +{
> +	camera_ = camera->shared_from_this();
> +	data_ = data;
> +}
> +
> +void IPU3CameraConfiguration::adjustStream(unsigned int index, bool scale)
> +{
> +	StreamConfiguration &cfg = config_[index];
> +
> +	/* The only pixel format the driver supports is NV12. */
> +	cfg.pixelFormat = V4L2_PIX_FMT_NV12;
> +
> +	if (scale) {
> +		/*
> +		 * Provide a suitable default that matches the sensor aspect
> +		 * ratio.
> +		 */
> +		if (!cfg.size.width || !cfg.size.height) {
> +			cfg.size.width = 1280;
> +			cfg.size.height = 1280 * sensorFormat_.size.height
> +					/ sensorFormat_.size.width;
> +		}
> +
> +		/*
> +		 * \todo: Clamp the size to the hardware bounds when we will
> +		 * figure them out.
> +		 *
> +		 * \todo: Handle the scaler (BDS) restrictions. The BDS can
> +		 * only scale with the same factor in both directions, and the
> +		 * scaling factor is limited to a multiple of 1/32. At the
> +		 * moment the ImgU driver hides these constraints by applying
> +		 * additional cropping, this should be fixed on the driver
> +		 * side, and cropping should be exposed to us.
> +		 */
> +	} else {
> +		/*
> +		 * \todo: Properly support cropping when the ImgU driver
> +		 * interface will be cleaned up.
> +		 */
> +		cfg.size = sensorFormat_.size;
> +	}
> +
> +	/*
> +	 * Clamp the size to match the ImgU alignment constraints. The width
> +	 * shall be a multiple of 8 pixels and the height a multiple of 4
> +	 * pixels.
> +	 */
> +	if (cfg.size.width % 8 || cfg.size.height % 4) {
> +		cfg.size.width &= ~7;
> +		cfg.size.height &= ~3;
> +	}
> +
> +	cfg.bufferCount = IPU3_BUFFER_COUNT;
> +}
> +
> +CameraConfiguration::Status IPU3CameraConfiguration::validate()
> +{
> +	const CameraSensor *sensor = data_->cio2_.sensor_;
> +	Status status = Valid;
> +
> +	if (config_.empty())
> +		return Invalid;
> +
> +	/* Cap the number of entries to the available streams. */
> +	if (config_.size() > 2) {
> +		config_.resize(2);
> +		status = Adjusted;
> +	}
> +
> +	/*
> +	 * Select the sensor format by collecting the maximum width and height
> +	 * and picking the closest larger match, as the IPU3 can downscale
> +	 * only. If no resolution is requested for any stream, or if no sensor
> +	 * resolution is large enough, pick the largest one.
> +	 */
> +	Size size = {};
> +
> +	for (const StreamConfiguration &cfg : config_) {
> +		if (cfg.size.width > size.width)
> +			size.width = cfg.size.width;
> +		if (cfg.size.height > size.height)
> +			size.height = cfg.size.height;
> +	}
> +
> +	if (!size.width || !size.height)
> +		size = sensor->resolution();
> +
> +	sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,
> +					    MEDIA_BUS_FMT_SGBRG10_1X10,
> +					    MEDIA_BUS_FMT_SGRBG10_1X10,
> +					    MEDIA_BUS_FMT_SRGGB10_1X10 },
> +					  size);
> +	if (!sensorFormat_.size.width || !sensorFormat_.size.height)
> +		sensorFormat_.size = sensor->resolution();
> +
> +	/*
> +	 * Verify and update all configuration entries, and assign a stream to
> +	 * each of them. The viewfinder stream can scale, while the output
> +	 * stream can crop only, so select the output stream when the requested
> +	 * resolution is equal to the sensor resolution, and the viewfinder
> +	 * stream otherwise.
> +	 */
> +	std::set<const IPU3Stream *> availableStreams = {
> +		&data_->outStream_,
> +		&data_->vfStream_,
> +	};
> +
> +	streams_.clear();
> +	streams_.reserve(config_.size());
> +
> +	for (unsigned int i = 0; i < config_.size(); ++i) {
> +		StreamConfiguration &cfg = config_[i];
> +		const unsigned int pixelFormat = cfg.pixelFormat;
> +		const Size size = cfg.size;
> +		const IPU3Stream *stream;
> +
> +		if (cfg.size == sensorFormat_.size)
> +			stream = &data_->outStream_;
> +		else
> +			stream = &data_->vfStream_;
> +
> +		if (availableStreams.find(stream) == availableStreams.end())
> +			stream = *availableStreams.begin();
> +
> +		LOG(IPU3, Debug)
> +			<< "Assigned '" << stream->name_ << "' to stream " << i;
> +
> +		bool scale = stream == &data_->vfStream_;
> +		adjustStream(i, scale);
> +
> +		if (cfg.pixelFormat != pixelFormat || cfg.size != size) {
> +			LOG(IPU3, Debug)
> +				<< "Stream " << i << " configuration adjusted to "
> +				<< cfg.toString();
> +			status = Adjusted;
> +		}
> +
> +		streams_.push_back(stream);
> +		availableStreams.erase(stream);
> +	}
> +
> +	return status;
> +}
> +
>  PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)
>  	: PipelineHandler(manager), cio2MediaDev_(nullptr), imguMediaDev_(nullptr)
>  {
> @@ -211,12 +383,14 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>  	const StreamRoles &roles)
>  {
>  	IPU3CameraData *data = cameraData(camera);
> -	CameraConfiguration *config = new CameraConfiguration();
> +	IPU3CameraConfiguration *config;
>  	std::set<IPU3Stream *> streams = {
>  		&data->outStream_,
>  		&data->vfStream_,
>  	};
>  
> +	config = new IPU3CameraConfiguration(camera, data);
> +
>  	for (const StreamRole role : roles) {
>  		StreamConfiguration cfg = {};
>  		IPU3Stream *stream = nullptr;
> @@ -296,71 +470,25 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>  
>  		streams.erase(stream);
>  
> -		cfg.pixelFormat = V4L2_PIX_FMT_NV12;
> -		cfg.bufferCount = IPU3_BUFFER_COUNT;
> -
>  		config->addConfiguration(cfg);
>  	}
>  
> +	config->validate();
> +
>  	return config;
>  }
>  
> -int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *config)
> +int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  {
> +	IPU3CameraConfiguration *config =
> +		static_cast<IPU3CameraConfiguration *>(c);
>  	IPU3CameraData *data = cameraData(camera);
>  	IPU3Stream *outStream = &data->outStream_;
>  	IPU3Stream *vfStream = &data->vfStream_;
>  	CIO2Device *cio2 = &data->cio2_;
>  	ImgUDevice *imgu = data->imgu_;
> -	Size sensorSize = {};
>  	int ret;
>  
> -	outStream->active_ = false;
> -	vfStream->active_ = false;
> -	for (StreamConfiguration &cfg : *config) {
> -		/*
> -		 * Pick a stream for the configuration entry.
> -		 * \todo: This is a naive temporary implementation that will be
> -		 * reworked when implementing camera configuration validation.
> -		 */
> -		IPU3Stream *stream = vfStream->active_ ? outStream : vfStream;
> -
> -		/*
> -		 * 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.size.width % 8 || cfg.size.height % 4) {
> -			LOG(IPU3, Error)
> -				<< "Invalid stream size: bad alignment";
> -			return -EINVAL;
> -		}
> -
> -		const Size &resolution = cio2->sensor_->resolution();
> -		if (cfg.size.width > resolution.width ||
> -		    cfg.size.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.size.width > sensorSize.width)
> -			sensorSize.width = cfg.size.width;
> -		if (cfg.size.height > sensorSize.height)
> -			sensorSize.height = cfg.size.height;
> -
> -		stream->active_ = true;
> -		cfg.setStream(stream);
> -	}
> -
>  	/*
>  	 * \todo: Enable links selectively based on the requested streams.
>  	 * As of now, enable all links unconditionally.
> @@ -373,6 +501,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *config)
>  	 * Pass the requested stream size to the CIO2 unit and get back the
>  	 * adjusted format to be propagated to the ImgU output devices.
>  	 */
> +	const Size &sensorSize = config->sensorFormat().size;
>  	V4L2DeviceFormat cio2Format = {};
>  	ret = cio2->configure(sensorSize, &cio2Format);
>  	if (ret)
> @@ -383,8 +512,22 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *config)
>  		return ret;
>  
>  	/* Apply the format to the configured streams output devices. */
> -	for (StreamConfiguration &cfg : *config) {
> -		IPU3Stream *stream = static_cast<IPU3Stream *>(cfg.stream());
> +	outStream->active_ = false;
> +	vfStream->active_ = false;
> +
> +	for (unsigned int i = 0; i < config->size(); ++i) {
> +		/*
> +		 * Use a const_cast<> here instead of storing a mutable stream
> +		 * pointer in the configuration to let the compiler catch
> +		 * unwanted modifications of camera data in the configuration
> +		 * validate() implementation.
> +		 */
> +		IPU3Stream *stream = const_cast<IPU3Stream *>(config->streams()[i]);
> +		StreamConfiguration &cfg = (*config)[i];
> +
> +		stream->active_ = true;
> +		cfg.setStream(stream);
> +
>  		ret = imgu->configureOutput(stream->device_, cfg);
>  		if (ret)
>  			return ret;
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index ec590a382751..b257f6534602 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -5,6 +5,7 @@
>   * rkisp1.cpp - Pipeline handler for Rockchip ISP1
>   */
>  
> +#include <algorithm>
>  #include <iomanip>
>  #include <memory>
>  #include <vector>
> @@ -45,6 +46,29 @@ public:
>  	CameraSensor *sensor_;
>  };
>  
> +class RkISP1CameraConfiguration : public CameraConfiguration
> +{
> +public:
> +	RkISP1CameraConfiguration(Camera *camera, RkISP1CameraData *data);
> +
> +	Status validate() override;
> +
> +	const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
> +
> +private:
> +	static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
> +
> +	/*
> +	 * The RkISP1CameraData instance is guaranteed to be valid as long as the
> +	 * corresponding Camera instance is valid. In order to borrow a
> +	 * reference to the camera data, store a new reference to the camera.
> +	 */
> +	std::shared_ptr<Camera> camera_;
> +	const RkISP1CameraData *data_;
> +
> +	V4L2SubdeviceFormat sensorFormat_;
> +};
> +
>  class PipelineHandlerRkISP1 : public PipelineHandler
>  {
>  public:
> @@ -68,8 +92,6 @@ public:
>  	bool match(DeviceEnumerator *enumerator) override;
>  
>  private:
> -	static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
> -
>  	RkISP1CameraData *cameraData(const Camera *camera)
>  	{
>  		return static_cast<RkISP1CameraData *>(
> @@ -88,6 +110,95 @@ private:
>  	Camera *activeCamera_;
>  };
>  
> +RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
> +						     RkISP1CameraData *data)
> +	: CameraConfiguration()
> +{
> +	camera_ = camera->shared_from_this();
> +	data_ = data;
> +}
> +
> +CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> +{
> +	static const std::array<unsigned int, 8> formats{
> +		V4L2_PIX_FMT_YUYV,
> +		V4L2_PIX_FMT_YVYU,
> +		V4L2_PIX_FMT_VYUY,
> +		V4L2_PIX_FMT_NV16,
> +		V4L2_PIX_FMT_NV61,
> +		V4L2_PIX_FMT_NV21,
> +		V4L2_PIX_FMT_NV12,
> +		V4L2_PIX_FMT_GREY,
> +	};
> +
> +	const CameraSensor *sensor = data_->sensor_;
> +	Status status = Valid;
> +
> +	if (config_.empty())
> +		return Invalid;
> +
> +	/* Cap the number of entries to the available streams. */
> +	if (config_.size() > 1) {
> +		config_.resize(1);
> +		status = Adjusted;
> +	}
> +
> +	StreamConfiguration &cfg = config_[0];
> +
> +	/* Adjust the pixel format. */
> +	if (std::find(formats.begin(), formats.end(), cfg.pixelFormat) ==
> +	    formats.end()) {
> +		LOG(RkISP1, Debug) << "Adjusting format to NV12";
> +		cfg.pixelFormat = V4L2_PIX_FMT_NV12;
> +		status = Adjusted;
> +	}
> +
> +	/* Select the sensor format. */
> +	sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,
> +					    MEDIA_BUS_FMT_SGBRG12_1X12,
> +					    MEDIA_BUS_FMT_SGRBG12_1X12,
> +					    MEDIA_BUS_FMT_SRGGB12_1X12,
> +					    MEDIA_BUS_FMT_SBGGR10_1X10,
> +					    MEDIA_BUS_FMT_SGBRG10_1X10,
> +					    MEDIA_BUS_FMT_SGRBG10_1X10,
> +					    MEDIA_BUS_FMT_SRGGB10_1X10,
> +					    MEDIA_BUS_FMT_SBGGR8_1X8,
> +					    MEDIA_BUS_FMT_SGBRG8_1X8,
> +					    MEDIA_BUS_FMT_SGRBG8_1X8,
> +					    MEDIA_BUS_FMT_SRGGB8_1X8 },
> +					  cfg.size);
> +	if (!sensorFormat_.size.width || !sensorFormat_.size.height)
> +		sensorFormat_.size = sensor->resolution();
> +
> +	/*
> +	 * Provide a suitable default that matches the sensor aspect
> +	 * ratio and clamp the size to the hardware bounds.
> +	 *
> +	 * \todo: Check the hardware alignment constraints.
> +	 */
> +	const Size size = cfg.size;
> +
> +	if (!cfg.size.width || !cfg.size.height) {
> +		cfg.size.width = 1280;
> +		cfg.size.height = 1280 * sensorFormat_.size.height
> +				/ sensorFormat_.size.width;
> +	}
> +
> +	cfg.size.width = std::max(32U, std::min(4416U, cfg.size.width));
> +	cfg.size.height = std::max(16U, std::min(3312U, cfg.size.height));
> +
> +	if (cfg.size != size) {
> +		LOG(RkISP1, Debug)
> +			<< "Adjusting size from " << size.toString()
> +			<< " to " << cfg.size.toString();
> +		status = Adjusted;
> +	}
> +
> +	cfg.bufferCount = RKISP1_BUFFER_COUNT;
> +
> +	return status;
> +}
> +
>  PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
>  	: PipelineHandler(manager), dphy_(nullptr), isp_(nullptr),
>  	  video_(nullptr)
> @@ -109,37 +220,31 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
>  	const StreamRoles &roles)
>  {
>  	RkISP1CameraData *data = cameraData(camera);
> -	CameraConfiguration *config = new CameraConfiguration();
> +	CameraConfiguration *config = new RkISP1CameraConfiguration(camera, data);
>  
>  	if (!roles.empty()) {
>  		StreamConfiguration cfg{};
>  
>  		cfg.pixelFormat = V4L2_PIX_FMT_NV12;
>  		cfg.size = data->sensor_->resolution();
> -		cfg.bufferCount = RKISP1_BUFFER_COUNT;
>  
>  		config->addConfiguration(cfg);
>  	}
>  
> +	config->validate();
> +
>  	return config;
>  }
>  
> -int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *config)
> +int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  {
> +	RkISP1CameraConfiguration *config =
> +		static_cast<RkISP1CameraConfiguration *>(c);
>  	RkISP1CameraData *data = cameraData(camera);
>  	StreamConfiguration &cfg = (*config)[0];
>  	CameraSensor *sensor = data->sensor_;
>  	int ret;
>  
> -	/* Verify the configuration. */
> -	const Size &resolution = sensor->resolution();
> -	if (cfg.size.width > resolution.width ||
> -	    cfg.size.height > resolution.height) {
> -		LOG(RkISP1, Error)
> -			<< "Invalid stream size: larger than sensor resolution";
> -		return -EINVAL;
> -	}
> -
>  	/*
>  	 * Configure the sensor links: enable the link corresponding to this
>  	 * camera and disable all the other sensor links.
> @@ -170,21 +275,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *config
>  	 * Configure the format on the sensor output and propagate it through
>  	 * the pipeline.
>  	 */
> -	V4L2SubdeviceFormat format;
> -	format = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,
> -				     MEDIA_BUS_FMT_SGBRG12_1X12,
> -				     MEDIA_BUS_FMT_SGRBG12_1X12,
> -				     MEDIA_BUS_FMT_SRGGB12_1X12,
> -				     MEDIA_BUS_FMT_SBGGR10_1X10,
> -				     MEDIA_BUS_FMT_SGBRG10_1X10,
> -				     MEDIA_BUS_FMT_SGRBG10_1X10,
> -				     MEDIA_BUS_FMT_SRGGB10_1X10,
> -				     MEDIA_BUS_FMT_SBGGR8_1X8,
> -				     MEDIA_BUS_FMT_SGBRG8_1X8,
> -				     MEDIA_BUS_FMT_SGRBG8_1X8,
> -				     MEDIA_BUS_FMT_SRGGB8_1X8 },
> -				   cfg.size);
> -
> +	V4L2SubdeviceFormat format = config->sensorFormat();
>  	LOG(RkISP1, Debug) << "Configuring sensor with " << format.toString();
>  
>  	ret = sensor->setFormat(&format);
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index c20467766ed0..286d19b0af01 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -39,6 +39,14 @@ public:
>  	Stream stream_;
>  };
>  
> +class UVCCameraConfiguration : public CameraConfiguration
> +{
> +public:
> +	UVCCameraConfiguration();
> +
> +	Status validate() override;
> +};
> +
>  class PipelineHandlerUVC : public PipelineHandler
>  {
>  public:
> @@ -68,6 +76,45 @@ private:
>  	}
>  };
>  
> +UVCCameraConfiguration::UVCCameraConfiguration()
> +	: CameraConfiguration()
> +{
> +}
> +
> +CameraConfiguration::Status UVCCameraConfiguration::validate()
> +{
> +	Status status = Valid;
> +
> +	if (config_.empty())
> +		return Invalid;
> +
> +	/* Cap the number of entries to the available streams. */
> +	if (config_.size() > 1) {
> +		config_.resize(1);
> +		status = Adjusted;
> +	}
> +
> +	StreamConfiguration &cfg = config_[0];
> +
> +	/* \todo: Validate the configuration against the device capabilities. */
> +	const unsigned int pixelFormat = cfg.pixelFormat;
> +	const Size size = cfg.size;
> +
> +	cfg.pixelFormat = V4L2_PIX_FMT_YUYV;
> +	cfg.size = { 640, 480 };
> +
> +	if (cfg.pixelFormat != pixelFormat || cfg.size != size) {
> +		LOG(UVC, Debug)
> +			<< "Adjusting configuration from " << cfg.toString()
> +			<< " to " << cfg.size.toString() << "-YUYV";
> +		status = Adjusted;
> +	}
> +
> +	cfg.bufferCount = 4;
> +
> +	return status;
> +}
> +
>  PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
>  	: PipelineHandler(manager)
>  {
> @@ -76,10 +123,10 @@ PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
>  CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,
>  	const StreamRoles &roles)
>  {
> -	CameraConfiguration *config = new CameraConfiguration();
> +	CameraConfiguration *config = new UVCCameraConfiguration();
>  
>  	if (!roles.empty()) {
> -		StreamConfiguration cfg;
> +		StreamConfiguration cfg{};
>  
>  		cfg.pixelFormat = V4L2_PIX_FMT_YUYV;
>  		cfg.size = { 640, 480 };
> @@ -88,6 +135,8 @@ CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,
>  		config->addConfiguration(cfg);
>  	}
>  
> +	config->validate();
> +
>  	return config;
>  }
>  
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index 5575880cdbdf..fd48d9b4ab39 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -5,6 +5,8 @@
>   * vimc.cpp - Pipeline handler for the vimc device
>   */
>  
> +#include <algorithm>
> +
>  #include <libcamera/camera.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
> @@ -39,6 +41,14 @@ public:
>  	Stream stream_;
>  };
>  
> +class VimcCameraConfiguration : public CameraConfiguration
> +{
> +public:
> +	VimcCameraConfiguration();
> +
> +	Status validate() override;
> +};
> +
>  class PipelineHandlerVimc : public PipelineHandler
>  {
>  public:
> @@ -68,6 +78,57 @@ private:
>  	}
>  };
>  
> +VimcCameraConfiguration::VimcCameraConfiguration()
> +	: CameraConfiguration()
> +{
> +}
> +
> +CameraConfiguration::Status VimcCameraConfiguration::validate()
> +{
> +	static const std::array<unsigned int, 3> formats{
> +		V4L2_PIX_FMT_BGR24,
> +		V4L2_PIX_FMT_RGB24,
> +		V4L2_PIX_FMT_ARGB32,
> +	};
> +
> +	Status status = Valid;
> +
> +	if (config_.empty())
> +		return Invalid;
> +
> +	/* Cap the number of entries to the available streams. */
> +	if (config_.size() > 1) {
> +		config_.resize(1);
> +		status = Adjusted;
> +	}
> +
> +	StreamConfiguration &cfg = config_[0];
> +
> +	/* Adjust the pixel format. */
> +	if (std::find(formats.begin(), formats.end(), cfg.pixelFormat) ==
> +	    formats.end()) {
> +		LOG(VIMC, Debug) << "Adjusting format to RGB24";
> +		cfg.pixelFormat = V4L2_PIX_FMT_RGB24;
> +		status = Adjusted;
> +	}
> +
> +	/* Clamp the size based on the device limits. */
> +	const Size size = cfg.size;
> +
> +	cfg.size.width = std::max(16U, std::min(4096U, cfg.size.width));
> +	cfg.size.height = std::max(16U, std::min(2160U, cfg.size.height));
> +
> +	if (cfg.size != size) {
> +		LOG(VIMC, Debug)
> +			<< "Adjusting size to " << cfg.size.toString();
> +		status = Adjusted;
> +	}
> +
> +	cfg.bufferCount = 4;
> +
> +	return status;
> +}
> +
>  PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)
>  	: PipelineHandler(manager)
>  {
> @@ -76,10 +137,10 @@ PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)
>  CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
>  	const StreamRoles &roles)
>  {
> -	CameraConfiguration *config = new CameraConfiguration();
> +	CameraConfiguration *config = new VimcCameraConfiguration();
>  
>  	if (!roles.empty()) {
> -		StreamConfiguration cfg;
> +		StreamConfiguration cfg{};
>  
>  		cfg.pixelFormat = V4L2_PIX_FMT_RGB24;
>  		cfg.size = { 640, 480 };
> @@ -88,6 +149,8 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
>  		config->addConfiguration(cfg);
>  	}
>  
> +	config->validate();
> +
>  	return config;
>  }
>  
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index de46e98880a2..dd56907d817e 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -248,17 +248,14 @@ void PipelineHandler::unlock()
>   * is the Camera class which will receive configuration to apply from the
>   * application.
>   *
> - * Each pipeline handler implementation is responsible for validating
> - * that the configuration requested in \a config can be achieved
> - * exactly. Any difference in pixel format, frame size or any other
> - * parameter shall result in the -EINVAL error being returned, and no
> - * change in configuration being applied to the pipeline. If
> - * configuration of a subset of the streams can't be satisfied, the
> - * whole configuration is considered invalid.
> + * The configuration is guaranteed to have been validated with
> + * CameraConfiguration::valid(). The pipeline handler implementation shall not
> + * perform further validation and may rely on any custom field stored in its
> + * custom CameraConfiguration derived class.
>   *
> - * Once the configuration is validated and the camera configured, the pipeline
> - * handler shall associate a Stream instance to each StreamConfiguration entry
> - * in the CameraConfiguration with the StreamConfiguration::setStream() method.
> + * When configuring the camera the pipeline handler shall associate a Stream
> + * instance to each StreamConfiguration entry in the CameraConfiguration using
> + * the StreamConfiguration::setStream() method.
>   *
>   * \return 0 on success or a negative error code otherwise
>   */
> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> index f640e80fbaf3..526b44e3a0fc 100644
> --- a/test/camera/capture.cpp
> +++ b/test/camera/capture.cpp
> @@ -45,7 +45,7 @@ protected:
>  		CameraTest::init();
>  
>  		config_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> -		if (!config_) {
> +		if (!config_ || config_->size() != 1) {
>  			cout << "Failed to generate default configuration" << endl;
>  			CameraTest::cleanup();
>  			return TestFail;
> @@ -58,11 +58,6 @@ protected:
>  	{
>  		StreamConfiguration &cfg = (*config_)[0];
>  
> -		if (!config_->isValid()) {
> -			cout << "Failed to read default configuration" << endl;
> -			return TestFail;
> -		}
> -
>  		if (camera_->acquire()) {
>  			cout << "Failed to acquire the camera" << endl;
>  			return TestFail;
> diff --git a/test/camera/configuration_default.cpp b/test/camera/configuration_default.cpp
> index d5cefc1127c9..81055da1d513 100644
> --- a/test/camera/configuration_default.cpp
> +++ b/test/camera/configuration_default.cpp
> @@ -22,7 +22,7 @@ protected:
>  
>  		/* Test asking for configuration for a video stream. */
>  		config = camera_->generateConfiguration({ StreamRole::VideoRecording });
> -		if (!config || !config->isValid()) {
> +		if (!config || config->size() != 1) {
>  			cout << "Default configuration invalid" << endl;
>  			delete config;
>  			return TestFail;
> @@ -35,7 +35,7 @@ protected:
>  		 * stream roles returns an empty camera configuration.
>  		 */
>  		config = camera_->generateConfiguration({});
> -		if (!config || config->isValid()) {
> +		if (!config || config->size() != 0) {
>  			cout << "Failed to retrieve configuration for empty roles list"
>  			     << endl;
>  			delete config;
> diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp
> index ca911f459c32..398a154cec69 100644
> --- a/test/camera/configuration_set.cpp
> +++ b/test/camera/configuration_set.cpp
> @@ -21,7 +21,7 @@ protected:
>  		CameraTest::init();
>  
>  		config_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> -		if (!config_) {
> +		if (!config_ || config_->size() != 1) {
>  			cout << "Failed to generate default configuration" << endl;
>  			CameraTest::cleanup();
>  			return TestFail;
> @@ -34,11 +34,6 @@ protected:
>  	{
>  		StreamConfiguration &cfg = (*config_)[0];
>  
> -		if (!config_->isValid()) {
> -			cout << "Failed to read default configuration" << endl;
> -			return TestFail;
> -		}
> -
>  		if (camera_->acquire()) {
>  			cout << "Failed to acquire the camera" << endl;
>  			return TestFail;
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Niklas,

On Sat, May 18, 2019 at 06:52:57PM +0200, Niklas Söderlund wrote:
> On 2019-05-18 02:06:16 +0300, Laurent Pinchart wrote:
> > The CameraConfiguration class implements a simple storage of
> > StreamConfiguration with internal validation limited to verifying that
> > the stream configurations are not empty. Extend this mechanism by
> > implementing a smart validate() method backed by pipeline handlers.
> > 
> > This new mechanism changes the semantics of the camera configuration.
> > The Camera::generateConfiguration() operation still generates a default
> > configuration based on roles, but now also supports generating empty
> > configurations to be filled by applications. Applications can inspect
> > the configuration, optionally modify it, and validate it. The validation
> > implements "try" semantics and adjusts invalid configurations instead of
> > rejecting them completely. Applications then decide whether to accept
> > the modified configuration, or try again with a different set of
> > parameters. Once the configuration is valid, it is passed to
> > Camera::configure(), and pipeline handlers are guaranteed that the
> > configuration they receive is valid.
> > 
> > A reference to the Camera may need to be stored in the
> > CameraConfiguration derived classes in order to access it from their
> > validate() implementation. This must be stored as a std::shared_ptr<> as
> > the CameraConfiguration instances belong to applications. In order to
> > make this possible, make the Camera class inherit from
> > std::shared_from_this<>.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/libcamera/camera.h               |  16 +-
> >  src/cam/main.cpp                         |   2 +-
> >  src/libcamera/camera.cpp                 |  90 +++++---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp     | 255 ++++++++++++++++++-----
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 149 ++++++++++---
> >  src/libcamera/pipeline/uvcvideo.cpp      |  53 ++++-
> >  src/libcamera/pipeline/vimc.cpp          |  67 +++++-
> >  src/libcamera/pipeline_handler.cpp       |  17 +-
> >  test/camera/capture.cpp                  |   7 +-
> >  test/camera/configuration_default.cpp    |   4 +-
> >  test/camera/configuration_set.cpp        |   7 +-
> >  11 files changed, 521 insertions(+), 146 deletions(-)
> > 
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 841e8fc505b9..276bf1fb1887 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -25,30 +25,38 @@ class Request;
> >  class CameraConfiguration
> >  {
> >  public:
> > +	enum Status {
> > +		Valid,
> > +		Adjusted,
> > +		Invalid,
> > +	};
> > +
> >  	using iterator = std::vector<StreamConfiguration>::iterator;
> >  	using const_iterator = std::vector<StreamConfiguration>::const_iterator;
> >  
> > -	CameraConfiguration();
> > +	virtual ~CameraConfiguration();
> >  
> >  	void addConfiguration(const StreamConfiguration &cfg);
> > +	virtual Status validate() = 0;
> >  
> >  	iterator begin();
> >  	iterator end();
> >  	const_iterator begin() const;
> >  	const_iterator end() const;
> >  
> > -	bool isValid() const;
> >  	bool isEmpty() const;
> >  	std::size_t size() const;
> >  
> >  	StreamConfiguration &operator[](unsigned int index);
> >  	const StreamConfiguration &operator[](unsigned int index) const;
> >  
> > -private:
> > +protected:
> > +	CameraConfiguration();
> > +
> >  	std::vector<StreamConfiguration> config_;
> >  };
> >  
> > -class Camera final
> > +class Camera final : public std::enable_shared_from_this<Camera>
> >  {
> >  public:
> >  	static std::shared_ptr<Camera> create(PipelineHandler *pipe,
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index 7a1b332f68c7..a962f94c8f59 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -116,7 +116,7 @@ static CameraConfiguration *prepareCameraConfig()
> >  	}
> >  
> >  	CameraConfiguration *config = camera->generateConfiguration(roles);
> > -	if (!config || !config->isValid()) {
> > +	if (!config || config->size() != roles.size()) {
> >  		std::cerr << "Failed to get default stream configuration"
> >  			  << std::endl;
> >  		delete config;
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 115cdb1c024b..572939956a34 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -52,6 +52,28 @@ LOG_DECLARE_CATEGORY(Camera)
> >   * operator[](int) returns a reference to the StreamConfiguration based on its
> >   * insertion index. Accessing a stream configuration with an invalid index
> >   * results in undefined behaviour.
> > + *
> > + * CameraConfiguration instances are retrieved from the camera with
> > + * Camera::generateConfiguration(). Applications may then inspect the
> > + * configuration, modify it, and possibly add new stream configuration entries
> > + * with addConfiguration(). Once the camera configuration satisfies the
> > + * application, it shall be validated by a call to validate(). The validation
> > + * implements "try" semantics: it adjusts invalid configurations to the closest
> > + * achievable parameters instead of rejecting them completely. Applications
> > + * then decide whether to accept the modified configuration, or try again with
> > + * a different set of parameters. Once the configuration is valid, it is passed
> > + * to Camera::configure().
> > + */
> > +
> > +/**
> > + * \enum CameraConfiguration::Status
> > + * \brief Validity of a camera configuration
> > + * \var CameraConfiguration::Valid
> > + * The configuration is fully valid
> > + * \var CameraConfiguration::Adjusted
> > + * The configuration has been adjusted to a valid configuration
> > + * \var CameraConfiguration::Invalid
> > + * The configuration is invalid and can't be adjusted automatically
> >   */
> >  
> >  /**
> > @@ -73,6 +95,10 @@ CameraConfiguration::CameraConfiguration()
> >  {
> >  }
> >  
> > +CameraConfiguration::~CameraConfiguration()
> > +{
> > +}
> > +
> >  /**
> >   * \brief Add a stream configuration to the camera configuration
> >   * \param[in] cfg The stream configuration
> > @@ -82,6 +108,33 @@ void CameraConfiguration::addConfiguration(const StreamConfiguration &cfg)
> >  	config_.push_back(cfg);
> >  }
> >  
> > +/**
> > + * \fn CameraConfiguration::validate()
> > + * \brief Validate and possibly adjust the camera configuration
> > + *
> > + * This method adjusts the camera configuration to the closest valid
> > + * configuration and returns the validation status.
> > + *
> > + * \todo: Define exactly when to return each status code. Should stream
> > + * parameters set to 0 by the caller be adjusted without returning Adjusted ?
> > + * This would potentially be useful for applications but would get in the way
> > + * in Camera::configure(). Do we need an extra status code to signal this ?
> > + *
> > + * \todo: Handle validation of buffers count when refactoring the buffers API.
> > + *
> > + * \return A CameraConfiguration::Status value that describes the validation
> > + * status.
> > + * \retval CameraConfiguration::Invalid The configuration is invalid and can't
> > + * be adjusted. This may only occur in extreme cases such as when the
> > + * configuration is empty.
> > + * \retval CameraConfigutation::Adjusted The configuration has been adjusted
> > + * and is now valid. Parameters may have changed for any stream, and stream
> > + * configurations may have been removed. The caller shall check the
> > + * configuration carefully.
> > + * \retval CameraConfiguration::Valid The configuration was already valid and
> > + * hasn't been adjusted.
> > + */
> > +
> >  /**
> >   * \brief Retrieve an iterator to the first stream configuration in the
> >   * sequence
> > @@ -123,29 +176,6 @@ CameraConfiguration::const_iterator CameraConfiguration::end() const
> >  	return config_.end();
> >  }
> >  
> > -/**
> > - * \brief Check if the camera configuration is valid
> > - *
> > - * A camera configuration is deemed to be valid if it contains at least one
> > - * stream configuration and all stream configurations contain valid information.
> > - * Stream configurations are deemed to be valid if all fields are none zero.
> > - *
> > - * \return True if the configuration is valid
> > - */
> > -bool CameraConfiguration::isValid() const
> > -{
> > -	if (isEmpty())
> > -		return false;
> > -
> > -	for (const StreamConfiguration &cfg : config_) {
> > -		if (cfg.size.width == 0 || cfg.size.height == 0 ||
> > -		    cfg.pixelFormat == 0 || cfg.bufferCount == 0)
> > -			return false;
> > -	}
> > -
> > -	return true;
> > -}
> > -
> >  /**
> >   * \brief Check if the camera configuration is empty
> >   * \return True if the configuration is empty
> > @@ -194,6 +224,11 @@ const StreamConfiguration &CameraConfiguration::operator[](unsigned int index) c
> >  	return config_[index];
> >  }
> >  
> > +/**
> > + * \var CameraConfiguration::config_
> > + * \brief The vector of stream configurations
> > + */
> 
> I think this should go into an earlier patch in this series, right?

It could, the reason it's here is because the field was private and
became protected. Do you think it should still be moved ?

> > +
> >  /**
> >   * \class Camera
> >   * \brief Camera device
> > @@ -551,10 +586,9 @@ CameraConfiguration *Camera::generateConfiguration(const StreamRoles &roles)
> >   * The caller specifies which streams are to be involved and their configuration
> >   * by populating \a config.
> >   *
> > - * The easiest way to populate the array of config is to fetch an initial
> > - * configuration from the camera with generateConfiguration() and then change
> > - * the parameters to fit the caller's need and once all the streams parameters
> > - * are configured hand that over to configure() to actually setup the camera.
> > + * The configuration is created by generateConfiguration(), and adjusted by the
> > + * caller with CameraConfiguration::validate(). This method only accepts fully
> > + * valid configurations and returns an error if \a config is not valid.
> >   *
> >   * Exclusive access to the camera shall be ensured by a call to acquire() prior
> >   * to calling this function, otherwise an -EACCES error will be returned.
> > @@ -579,7 +613,7 @@ int Camera::configure(CameraConfiguration *config)
> >  	if (!stateBetween(CameraAcquired, CameraConfigured))
> >  		return -EACCES;
> >  
> > -	if (!config->isValid()) {
> > +	if (config->validate() != CameraConfiguration::Valid) {
> >  		LOG(Camera, Error)
> >  			<< "Can't configure camera with invalid configuration";
> >  		return -EINVAL;
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 8430e0591a41..a5e71832d3c2 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -164,6 +164,33 @@ public:
> >  	IPU3Stream vfStream_;
> >  };
> >  
> > +class IPU3CameraConfiguration : public CameraConfiguration
> > +{
> > +public:
> > +	IPU3CameraConfiguration(Camera *camera, IPU3CameraData *data);
> > +
> > +	Status validate() override;
> > +
> > +	const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
> > +	const std::vector<const IPU3Stream *> &streams() { return streams_; }
> > +
> > +private:
> > +	static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> > +
> > +	void adjustStream(unsigned int index, bool scale);
> > +
> > +	/*
> > +	 * The IPU3CameraData instance is guaranteed to be valid as long as the
> > +	 * corresponding Camera instance is valid. In order to borrow a
> > +	 * reference to the camera data, store a new reference to the camera.
> > +	 */
> > +	std::shared_ptr<Camera> camera_;
> > +	const IPU3CameraData *data_;
> > +
> > +	V4L2SubdeviceFormat sensorFormat_;
> > +	std::vector<const IPU3Stream *> streams_;
> > +};
> > +
> >  class PipelineHandlerIPU3 : public PipelineHandler
> >  {
> >  public:
> > @@ -186,8 +213,6 @@ public:
> >  	bool match(DeviceEnumerator *enumerator) override;
> >  
> >  private:
> > -	static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> > -
> >  	IPU3CameraData *cameraData(const Camera *camera)
> >  	{
> >  		return static_cast<IPU3CameraData *>(
> > @@ -202,6 +227,153 @@ private:
> >  	MediaDevice *imguMediaDev_;
> >  };
> >  
> > +IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera,
> > +						 IPU3CameraData *data)
> > +	: CameraConfiguration()
> > +{
> > +	camera_ = camera->shared_from_this();
> > +	data_ = data;
> > +}
> > +
> > +void IPU3CameraConfiguration::adjustStream(unsigned int index, bool scale)
> > +{
> > +	StreamConfiguration &cfg = config_[index];
> > +
> > +	/* The only pixel format the driver supports is NV12. */
> > +	cfg.pixelFormat = V4L2_PIX_FMT_NV12;
> > +
> > +	if (scale) {
> > +		/*
> > +		 * Provide a suitable default that matches the sensor aspect
> > +		 * ratio.
> > +		 */
> > +		if (!cfg.size.width || !cfg.size.height) {
> > +			cfg.size.width = 1280;
> > +			cfg.size.height = 1280 * sensorFormat_.size.height
> > +					/ sensorFormat_.size.width;
> > +		}
> > +
> > +		/*
> > +		 * \todo: Clamp the size to the hardware bounds when we will
> > +		 * figure them out.
> > +		 *
> > +		 * \todo: Handle the scaler (BDS) restrictions. The BDS can
> > +		 * only scale with the same factor in both directions, and the
> > +		 * scaling factor is limited to a multiple of 1/32. At the
> > +		 * moment the ImgU driver hides these constraints by applying
> > +		 * additional cropping, this should be fixed on the driver
> > +		 * side, and cropping should be exposed to us.
> > +		 */
> > +	} else {
> > +		/*
> > +		 * \todo: Properly support cropping when the ImgU driver
> > +		 * interface will be cleaned up.
> > +		 */
> > +		cfg.size = sensorFormat_.size;
> > +	}
> > +
> > +	/*
> > +	 * Clamp the size to match the ImgU alignment constraints. The width
> > +	 * shall be a multiple of 8 pixels and the height a multiple of 4
> > +	 * pixels.
> > +	 */
> > +	if (cfg.size.width % 8 || cfg.size.height % 4) {
> > +		cfg.size.width &= ~7;
> > +		cfg.size.height &= ~3;
> > +	}
> > +
> > +	cfg.bufferCount = IPU3_BUFFER_COUNT;
> > +}
> > +
> > +CameraConfiguration::Status IPU3CameraConfiguration::validate()
> > +{
> > +	const CameraSensor *sensor = data_->cio2_.sensor_;
> > +	Status status = Valid;
> > +
> > +	if (config_.empty())
> > +		return Invalid;
> > +
> > +	/* Cap the number of entries to the available streams. */
> > +	if (config_.size() > 2) {
> > +		config_.resize(2);
> > +		status = Adjusted;
> > +	}
> > +
> > +	/*
> > +	 * Select the sensor format by collecting the maximum width and height
> > +	 * and picking the closest larger match, as the IPU3 can downscale
> > +	 * only. If no resolution is requested for any stream, or if no sensor
> > +	 * resolution is large enough, pick the largest one.
> > +	 */
> > +	Size size = {};
> > +
> > +	for (const StreamConfiguration &cfg : config_) {
> > +		if (cfg.size.width > size.width)
> > +			size.width = cfg.size.width;
> > +		if (cfg.size.height > size.height)
> > +			size.height = cfg.size.height;
> > +	}
> > +
> > +	if (!size.width || !size.height)
> > +		size = sensor->resolution();
> > +
> > +	sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,
> > +					    MEDIA_BUS_FMT_SGBRG10_1X10,
> > +					    MEDIA_BUS_FMT_SGRBG10_1X10,
> > +					    MEDIA_BUS_FMT_SRGGB10_1X10 },
> > +					  size);
> > +	if (!sensorFormat_.size.width || !sensorFormat_.size.height)
> > +		sensorFormat_.size = sensor->resolution();
> > +
> > +	/*
> > +	 * Verify and update all configuration entries, and assign a stream to
> > +	 * each of them. The viewfinder stream can scale, while the output
> > +	 * stream can crop only, so select the output stream when the requested
> > +	 * resolution is equal to the sensor resolution, and the viewfinder
> > +	 * stream otherwise.
> > +	 */
> > +	std::set<const IPU3Stream *> availableStreams = {
> > +		&data_->outStream_,
> > +		&data_->vfStream_,
> > +	};
> > +
> > +	streams_.clear();
> > +	streams_.reserve(config_.size());
> > +
> > +	for (unsigned int i = 0; i < config_.size(); ++i) {
> > +		StreamConfiguration &cfg = config_[i];
> > +		const unsigned int pixelFormat = cfg.pixelFormat;
> > +		const Size size = cfg.size;
> > +		const IPU3Stream *stream;
> > +
> > +		if (cfg.size == sensorFormat_.size)
> > +			stream = &data_->outStream_;
> > +		else
> > +			stream = &data_->vfStream_;
> > +
> > +		if (availableStreams.find(stream) == availableStreams.end())
> > +			stream = *availableStreams.begin();
> > +
> > +		LOG(IPU3, Debug)
> > +			<< "Assigned '" << stream->name_ << "' to stream " << i;
> > +
> > +		bool scale = stream == &data_->vfStream_;
> > +		adjustStream(i, scale);
> > +
> > +		if (cfg.pixelFormat != pixelFormat || cfg.size != size) {
> > +			LOG(IPU3, Debug)
> > +				<< "Stream " << i << " configuration adjusted to "
> > +				<< cfg.toString();
> > +			status = Adjusted;
> > +		}
> > +
> > +		streams_.push_back(stream);
> > +		availableStreams.erase(stream);
> > +	}
> > +
> > +	return status;
> > +}
> > +
> >  PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)
> >  	: PipelineHandler(manager), cio2MediaDev_(nullptr), imguMediaDev_(nullptr)
> >  {
> > @@ -211,12 +383,14 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> >  	const StreamRoles &roles)
> >  {
> >  	IPU3CameraData *data = cameraData(camera);
> > -	CameraConfiguration *config = new CameraConfiguration();
> > +	IPU3CameraConfiguration *config;
> >  	std::set<IPU3Stream *> streams = {
> >  		&data->outStream_,
> >  		&data->vfStream_,
> >  	};
> >  
> > +	config = new IPU3CameraConfiguration(camera, data);
> > +
> >  	for (const StreamRole role : roles) {
> >  		StreamConfiguration cfg = {};
> >  		IPU3Stream *stream = nullptr;
> > @@ -296,71 +470,25 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> >  
> >  		streams.erase(stream);
> >  
> > -		cfg.pixelFormat = V4L2_PIX_FMT_NV12;
> > -		cfg.bufferCount = IPU3_BUFFER_COUNT;
> > -
> >  		config->addConfiguration(cfg);
> >  	}
> >  
> > +	config->validate();
> > +
> >  	return config;
> >  }
> >  
> > -int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *config)
> > +int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> >  {
> > +	IPU3CameraConfiguration *config =
> > +		static_cast<IPU3CameraConfiguration *>(c);
> >  	IPU3CameraData *data = cameraData(camera);
> >  	IPU3Stream *outStream = &data->outStream_;
> >  	IPU3Stream *vfStream = &data->vfStream_;
> >  	CIO2Device *cio2 = &data->cio2_;
> >  	ImgUDevice *imgu = data->imgu_;
> > -	Size sensorSize = {};
> >  	int ret;
> >  
> > -	outStream->active_ = false;
> > -	vfStream->active_ = false;
> > -	for (StreamConfiguration &cfg : *config) {
> > -		/*
> > -		 * Pick a stream for the configuration entry.
> > -		 * \todo: This is a naive temporary implementation that will be
> > -		 * reworked when implementing camera configuration validation.
> > -		 */
> > -		IPU3Stream *stream = vfStream->active_ ? outStream : vfStream;
> > -
> > -		/*
> > -		 * 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.size.width % 8 || cfg.size.height % 4) {
> > -			LOG(IPU3, Error)
> > -				<< "Invalid stream size: bad alignment";
> > -			return -EINVAL;
> > -		}
> > -
> > -		const Size &resolution = cio2->sensor_->resolution();
> > -		if (cfg.size.width > resolution.width ||
> > -		    cfg.size.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.size.width > sensorSize.width)
> > -			sensorSize.width = cfg.size.width;
> > -		if (cfg.size.height > sensorSize.height)
> > -			sensorSize.height = cfg.size.height;
> > -
> > -		stream->active_ = true;
> > -		cfg.setStream(stream);
> > -	}
> > -
> >  	/*
> >  	 * \todo: Enable links selectively based on the requested streams.
> >  	 * As of now, enable all links unconditionally.
> > @@ -373,6 +501,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *config)
> >  	 * Pass the requested stream size to the CIO2 unit and get back the
> >  	 * adjusted format to be propagated to the ImgU output devices.
> >  	 */
> > +	const Size &sensorSize = config->sensorFormat().size;
> >  	V4L2DeviceFormat cio2Format = {};
> >  	ret = cio2->configure(sensorSize, &cio2Format);
> >  	if (ret)
> > @@ -383,8 +512,22 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *config)
> >  		return ret;
> >  
> >  	/* Apply the format to the configured streams output devices. */
> > -	for (StreamConfiguration &cfg : *config) {
> > -		IPU3Stream *stream = static_cast<IPU3Stream *>(cfg.stream());
> > +	outStream->active_ = false;
> > +	vfStream->active_ = false;
> > +
> > +	for (unsigned int i = 0; i < config->size(); ++i) {
> > +		/*
> > +		 * Use a const_cast<> here instead of storing a mutable stream
> > +		 * pointer in the configuration to let the compiler catch
> > +		 * unwanted modifications of camera data in the configuration
> > +		 * validate() implementation.
> > +		 */
> > +		IPU3Stream *stream = const_cast<IPU3Stream *>(config->streams()[i]);
> > +		StreamConfiguration &cfg = (*config)[i];
> > +
> > +		stream->active_ = true;
> > +		cfg.setStream(stream);
> > +
> >  		ret = imgu->configureOutput(stream->device_, cfg);
> >  		if (ret)
> >  			return ret;
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index ec590a382751..b257f6534602 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -5,6 +5,7 @@
> >   * rkisp1.cpp - Pipeline handler for Rockchip ISP1
> >   */
> >  
> > +#include <algorithm>
> >  #include <iomanip>
> >  #include <memory>
> >  #include <vector>
> > @@ -45,6 +46,29 @@ public:
> >  	CameraSensor *sensor_;
> >  };
> >  
> > +class RkISP1CameraConfiguration : public CameraConfiguration
> > +{
> > +public:
> > +	RkISP1CameraConfiguration(Camera *camera, RkISP1CameraData *data);
> > +
> > +	Status validate() override;
> > +
> > +	const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
> > +
> > +private:
> > +	static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
> > +
> > +	/*
> > +	 * The RkISP1CameraData instance is guaranteed to be valid as long as the
> > +	 * corresponding Camera instance is valid. In order to borrow a
> > +	 * reference to the camera data, store a new reference to the camera.
> > +	 */
> > +	std::shared_ptr<Camera> camera_;
> > +	const RkISP1CameraData *data_;
> > +
> > +	V4L2SubdeviceFormat sensorFormat_;
> > +};
> > +
> >  class PipelineHandlerRkISP1 : public PipelineHandler
> >  {
> >  public:
> > @@ -68,8 +92,6 @@ public:
> >  	bool match(DeviceEnumerator *enumerator) override;
> >  
> >  private:
> > -	static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
> > -
> >  	RkISP1CameraData *cameraData(const Camera *camera)
> >  	{
> >  		return static_cast<RkISP1CameraData *>(
> > @@ -88,6 +110,95 @@ private:
> >  	Camera *activeCamera_;
> >  };
> >  
> > +RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
> > +						     RkISP1CameraData *data)
> > +	: CameraConfiguration()
> > +{
> > +	camera_ = camera->shared_from_this();
> > +	data_ = data;
> > +}
> > +
> > +CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> > +{
> > +	static const std::array<unsigned int, 8> formats{
> > +		V4L2_PIX_FMT_YUYV,
> > +		V4L2_PIX_FMT_YVYU,
> > +		V4L2_PIX_FMT_VYUY,
> > +		V4L2_PIX_FMT_NV16,
> > +		V4L2_PIX_FMT_NV61,
> > +		V4L2_PIX_FMT_NV21,
> > +		V4L2_PIX_FMT_NV12,
> > +		V4L2_PIX_FMT_GREY,
> > +	};
> > +
> > +	const CameraSensor *sensor = data_->sensor_;
> > +	Status status = Valid;
> > +
> > +	if (config_.empty())
> > +		return Invalid;
> > +
> > +	/* Cap the number of entries to the available streams. */
> > +	if (config_.size() > 1) {
> > +		config_.resize(1);
> > +		status = Adjusted;
> > +	}
> > +
> > +	StreamConfiguration &cfg = config_[0];
> > +
> > +	/* Adjust the pixel format. */
> > +	if (std::find(formats.begin(), formats.end(), cfg.pixelFormat) ==
> > +	    formats.end()) {
> > +		LOG(RkISP1, Debug) << "Adjusting format to NV12";
> > +		cfg.pixelFormat = V4L2_PIX_FMT_NV12;
> > +		status = Adjusted;
> > +	}
> > +
> > +	/* Select the sensor format. */
> > +	sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,
> > +					    MEDIA_BUS_FMT_SGBRG12_1X12,
> > +					    MEDIA_BUS_FMT_SGRBG12_1X12,
> > +					    MEDIA_BUS_FMT_SRGGB12_1X12,
> > +					    MEDIA_BUS_FMT_SBGGR10_1X10,
> > +					    MEDIA_BUS_FMT_SGBRG10_1X10,
> > +					    MEDIA_BUS_FMT_SGRBG10_1X10,
> > +					    MEDIA_BUS_FMT_SRGGB10_1X10,
> > +					    MEDIA_BUS_FMT_SBGGR8_1X8,
> > +					    MEDIA_BUS_FMT_SGBRG8_1X8,
> > +					    MEDIA_BUS_FMT_SGRBG8_1X8,
> > +					    MEDIA_BUS_FMT_SRGGB8_1X8 },
> > +					  cfg.size);
> > +	if (!sensorFormat_.size.width || !sensorFormat_.size.height)
> > +		sensorFormat_.size = sensor->resolution();
> > +
> > +	/*
> > +	 * Provide a suitable default that matches the sensor aspect
> > +	 * ratio and clamp the size to the hardware bounds.
> > +	 *
> > +	 * \todo: Check the hardware alignment constraints.
> > +	 */
> > +	const Size size = cfg.size;
> > +
> > +	if (!cfg.size.width || !cfg.size.height) {
> > +		cfg.size.width = 1280;
> > +		cfg.size.height = 1280 * sensorFormat_.size.height
> > +				/ sensorFormat_.size.width;
> > +	}
> > +
> > +	cfg.size.width = std::max(32U, std::min(4416U, cfg.size.width));
> > +	cfg.size.height = std::max(16U, std::min(3312U, cfg.size.height));
> > +
> > +	if (cfg.size != size) {
> > +		LOG(RkISP1, Debug)
> > +			<< "Adjusting size from " << size.toString()
> > +			<< " to " << cfg.size.toString();
> > +		status = Adjusted;
> > +	}
> > +
> > +	cfg.bufferCount = RKISP1_BUFFER_COUNT;
> > +
> > +	return status;
> > +}
> > +
> >  PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
> >  	: PipelineHandler(manager), dphy_(nullptr), isp_(nullptr),
> >  	  video_(nullptr)
> > @@ -109,37 +220,31 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
> >  	const StreamRoles &roles)
> >  {
> >  	RkISP1CameraData *data = cameraData(camera);
> > -	CameraConfiguration *config = new CameraConfiguration();
> > +	CameraConfiguration *config = new RkISP1CameraConfiguration(camera, data);
> >  
> >  	if (!roles.empty()) {
> >  		StreamConfiguration cfg{};
> >  
> >  		cfg.pixelFormat = V4L2_PIX_FMT_NV12;
> >  		cfg.size = data->sensor_->resolution();
> > -		cfg.bufferCount = RKISP1_BUFFER_COUNT;
> >  
> >  		config->addConfiguration(cfg);
> >  	}
> >  
> > +	config->validate();
> > +
> >  	return config;
> >  }
> >  
> > -int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *config)
> > +int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> >  {
> > +	RkISP1CameraConfiguration *config =
> > +		static_cast<RkISP1CameraConfiguration *>(c);
> >  	RkISP1CameraData *data = cameraData(camera);
> >  	StreamConfiguration &cfg = (*config)[0];
> >  	CameraSensor *sensor = data->sensor_;
> >  	int ret;
> >  
> > -	/* Verify the configuration. */
> > -	const Size &resolution = sensor->resolution();
> > -	if (cfg.size.width > resolution.width ||
> > -	    cfg.size.height > resolution.height) {
> > -		LOG(RkISP1, Error)
> > -			<< "Invalid stream size: larger than sensor resolution";
> > -		return -EINVAL;
> > -	}
> > -
> >  	/*
> >  	 * Configure the sensor links: enable the link corresponding to this
> >  	 * camera and disable all the other sensor links.
> > @@ -170,21 +275,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *config
> >  	 * Configure the format on the sensor output and propagate it through
> >  	 * the pipeline.
> >  	 */
> > -	V4L2SubdeviceFormat format;
> > -	format = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,
> > -				     MEDIA_BUS_FMT_SGBRG12_1X12,
> > -				     MEDIA_BUS_FMT_SGRBG12_1X12,
> > -				     MEDIA_BUS_FMT_SRGGB12_1X12,
> > -				     MEDIA_BUS_FMT_SBGGR10_1X10,
> > -				     MEDIA_BUS_FMT_SGBRG10_1X10,
> > -				     MEDIA_BUS_FMT_SGRBG10_1X10,
> > -				     MEDIA_BUS_FMT_SRGGB10_1X10,
> > -				     MEDIA_BUS_FMT_SBGGR8_1X8,
> > -				     MEDIA_BUS_FMT_SGBRG8_1X8,
> > -				     MEDIA_BUS_FMT_SGRBG8_1X8,
> > -				     MEDIA_BUS_FMT_SRGGB8_1X8 },
> > -				   cfg.size);
> > -
> > +	V4L2SubdeviceFormat format = config->sensorFormat();
> >  	LOG(RkISP1, Debug) << "Configuring sensor with " << format.toString();
> >  
> >  	ret = sensor->setFormat(&format);
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index c20467766ed0..286d19b0af01 100644
> > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > @@ -39,6 +39,14 @@ public:
> >  	Stream stream_;
> >  };
> >  
> > +class UVCCameraConfiguration : public CameraConfiguration
> > +{
> > +public:
> > +	UVCCameraConfiguration();
> > +
> > +	Status validate() override;
> > +};
> > +
> >  class PipelineHandlerUVC : public PipelineHandler
> >  {
> >  public:
> > @@ -68,6 +76,45 @@ private:
> >  	}
> >  };
> >  
> > +UVCCameraConfiguration::UVCCameraConfiguration()
> > +	: CameraConfiguration()
> > +{
> > +}
> > +
> > +CameraConfiguration::Status UVCCameraConfiguration::validate()
> > +{
> > +	Status status = Valid;
> > +
> > +	if (config_.empty())
> > +		return Invalid;
> > +
> > +	/* Cap the number of entries to the available streams. */
> > +	if (config_.size() > 1) {
> > +		config_.resize(1);
> > +		status = Adjusted;
> > +	}
> > +
> > +	StreamConfiguration &cfg = config_[0];
> > +
> > +	/* \todo: Validate the configuration against the device capabilities. */
> > +	const unsigned int pixelFormat = cfg.pixelFormat;
> > +	const Size size = cfg.size;
> > +
> > +	cfg.pixelFormat = V4L2_PIX_FMT_YUYV;
> > +	cfg.size = { 640, 480 };
> > +
> > +	if (cfg.pixelFormat != pixelFormat || cfg.size != size) {
> > +		LOG(UVC, Debug)
> > +			<< "Adjusting configuration from " << cfg.toString()
> > +			<< " to " << cfg.size.toString() << "-YUYV";
> > +		status = Adjusted;
> > +	}
> > +
> > +	cfg.bufferCount = 4;
> > +
> > +	return status;
> > +}
> > +
> >  PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
> >  	: PipelineHandler(manager)
> >  {
> > @@ -76,10 +123,10 @@ PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
> >  CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,
> >  	const StreamRoles &roles)
> >  {
> > -	CameraConfiguration *config = new CameraConfiguration();
> > +	CameraConfiguration *config = new UVCCameraConfiguration();
> >  
> >  	if (!roles.empty()) {
> > -		StreamConfiguration cfg;
> > +		StreamConfiguration cfg{};
> >  
> >  		cfg.pixelFormat = V4L2_PIX_FMT_YUYV;
> >  		cfg.size = { 640, 480 };
> > @@ -88,6 +135,8 @@ CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,
> >  		config->addConfiguration(cfg);
> >  	}
> >  
> > +	config->validate();
> > +
> >  	return config;
> >  }
> >  
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index 5575880cdbdf..fd48d9b4ab39 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -5,6 +5,8 @@
> >   * vimc.cpp - Pipeline handler for the vimc device
> >   */
> >  
> > +#include <algorithm>
> > +
> >  #include <libcamera/camera.h>
> >  #include <libcamera/request.h>
> >  #include <libcamera/stream.h>
> > @@ -39,6 +41,14 @@ public:
> >  	Stream stream_;
> >  };
> >  
> > +class VimcCameraConfiguration : public CameraConfiguration
> > +{
> > +public:
> > +	VimcCameraConfiguration();
> > +
> > +	Status validate() override;
> > +};
> > +
> >  class PipelineHandlerVimc : public PipelineHandler
> >  {
> >  public:
> > @@ -68,6 +78,57 @@ private:
> >  	}
> >  };
> >  
> > +VimcCameraConfiguration::VimcCameraConfiguration()
> > +	: CameraConfiguration()
> > +{
> > +}
> > +
> > +CameraConfiguration::Status VimcCameraConfiguration::validate()
> > +{
> > +	static const std::array<unsigned int, 3> formats{
> > +		V4L2_PIX_FMT_BGR24,
> > +		V4L2_PIX_FMT_RGB24,
> > +		V4L2_PIX_FMT_ARGB32,
> > +	};
> > +
> > +	Status status = Valid;
> > +
> > +	if (config_.empty())
> > +		return Invalid;
> > +
> > +	/* Cap the number of entries to the available streams. */
> > +	if (config_.size() > 1) {
> > +		config_.resize(1);
> > +		status = Adjusted;
> > +	}
> > +
> > +	StreamConfiguration &cfg = config_[0];
> > +
> > +	/* Adjust the pixel format. */
> > +	if (std::find(formats.begin(), formats.end(), cfg.pixelFormat) ==
> > +	    formats.end()) {
> > +		LOG(VIMC, Debug) << "Adjusting format to RGB24";
> > +		cfg.pixelFormat = V4L2_PIX_FMT_RGB24;
> > +		status = Adjusted;
> > +	}
> > +
> > +	/* Clamp the size based on the device limits. */
> > +	const Size size = cfg.size;
> > +
> > +	cfg.size.width = std::max(16U, std::min(4096U, cfg.size.width));
> > +	cfg.size.height = std::max(16U, std::min(2160U, cfg.size.height));
> > +
> > +	if (cfg.size != size) {
> > +		LOG(VIMC, Debug)
> > +			<< "Adjusting size to " << cfg.size.toString();
> > +		status = Adjusted;
> > +	}
> > +
> > +	cfg.bufferCount = 4;
> > +
> > +	return status;
> > +}
> > +
> >  PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)
> >  	: PipelineHandler(manager)
> >  {
> > @@ -76,10 +137,10 @@ PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)
> >  CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
> >  	const StreamRoles &roles)
> >  {
> > -	CameraConfiguration *config = new CameraConfiguration();
> > +	CameraConfiguration *config = new VimcCameraConfiguration();
> >  
> >  	if (!roles.empty()) {
> > -		StreamConfiguration cfg;
> > +		StreamConfiguration cfg{};
> >  
> >  		cfg.pixelFormat = V4L2_PIX_FMT_RGB24;
> >  		cfg.size = { 640, 480 };
> > @@ -88,6 +149,8 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
> >  		config->addConfiguration(cfg);
> >  	}
> >  
> > +	config->validate();
> > +
> >  	return config;
> >  }
> >  
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index de46e98880a2..dd56907d817e 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -248,17 +248,14 @@ void PipelineHandler::unlock()
> >   * is the Camera class which will receive configuration to apply from the
> >   * application.
> >   *
> > - * Each pipeline handler implementation is responsible for validating
> > - * that the configuration requested in \a config can be achieved
> > - * exactly. Any difference in pixel format, frame size or any other
> > - * parameter shall result in the -EINVAL error being returned, and no
> > - * change in configuration being applied to the pipeline. If
> > - * configuration of a subset of the streams can't be satisfied, the
> > - * whole configuration is considered invalid.
> > + * The configuration is guaranteed to have been validated with
> > + * CameraConfiguration::valid(). The pipeline handler implementation shall not
> > + * perform further validation and may rely on any custom field stored in its
> > + * custom CameraConfiguration derived class.
> >   *
> > - * Once the configuration is validated and the camera configured, the pipeline
> > - * handler shall associate a Stream instance to each StreamConfiguration entry
> > - * in the CameraConfiguration with the StreamConfiguration::setStream() method.
> > + * When configuring the camera the pipeline handler shall associate a Stream
> > + * instance to each StreamConfiguration entry in the CameraConfiguration using
> > + * the StreamConfiguration::setStream() method.
> >   *
> >   * \return 0 on success or a negative error code otherwise
> >   */
> > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> > index f640e80fbaf3..526b44e3a0fc 100644
> > --- a/test/camera/capture.cpp
> > +++ b/test/camera/capture.cpp
> > @@ -45,7 +45,7 @@ protected:
> >  		CameraTest::init();
> >  
> >  		config_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> > -		if (!config_) {
> > +		if (!config_ || config_->size() != 1) {
> >  			cout << "Failed to generate default configuration" << endl;
> >  			CameraTest::cleanup();
> >  			return TestFail;
> > @@ -58,11 +58,6 @@ protected:
> >  	{
> >  		StreamConfiguration &cfg = (*config_)[0];
> >  
> > -		if (!config_->isValid()) {
> > -			cout << "Failed to read default configuration" << endl;
> > -			return TestFail;
> > -		}
> > -
> >  		if (camera_->acquire()) {
> >  			cout << "Failed to acquire the camera" << endl;
> >  			return TestFail;
> > diff --git a/test/camera/configuration_default.cpp b/test/camera/configuration_default.cpp
> > index d5cefc1127c9..81055da1d513 100644
> > --- a/test/camera/configuration_default.cpp
> > +++ b/test/camera/configuration_default.cpp
> > @@ -22,7 +22,7 @@ protected:
> >  
> >  		/* Test asking for configuration for a video stream. */
> >  		config = camera_->generateConfiguration({ StreamRole::VideoRecording });
> > -		if (!config || !config->isValid()) {
> > +		if (!config || config->size() != 1) {
> >  			cout << "Default configuration invalid" << endl;
> >  			delete config;
> >  			return TestFail;
> > @@ -35,7 +35,7 @@ protected:
> >  		 * stream roles returns an empty camera configuration.
> >  		 */
> >  		config = camera_->generateConfiguration({});
> > -		if (!config || config->isValid()) {
> > +		if (!config || config->size() != 0) {
> >  			cout << "Failed to retrieve configuration for empty roles list"
> >  			     << endl;
> >  			delete config;
> > diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp
> > index ca911f459c32..398a154cec69 100644
> > --- a/test/camera/configuration_set.cpp
> > +++ b/test/camera/configuration_set.cpp
> > @@ -21,7 +21,7 @@ protected:
> >  		CameraTest::init();
> >  
> >  		config_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> > -		if (!config_) {
> > +		if (!config_ || config_->size() != 1) {
> >  			cout << "Failed to generate default configuration" << endl;
> >  			CameraTest::cleanup();
> >  			return TestFail;
> > @@ -34,11 +34,6 @@ protected:
> >  	{
> >  		StreamConfiguration &cfg = (*config_)[0];
> >  
> > -		if (!config_->isValid()) {
> > -			cout << "Failed to read default configuration" << endl;
> > -			return TestFail;
> > -		}
> > -
> >  		if (camera_->acquire()) {
> >  			cout << "Failed to acquire the camera" << endl;
> >  			return TestFail;