[libcamera-devel,v3,13/22] libcamera: pipeline: rkisp1: Add format validation for self path

9811 diff mbox series

Extend the format validation to work with both main and self paths. The
heuristics honors that the first stream in the configuration has the
highest priority while still examining both streams for a best match.

It is not possible to capture from the self path as the self stream is
not yet exposed to applications.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
* Changes since v2
- Fix spelling in commit message.
- Use Span<> instead of turning arrays to vectors.
- Keep data_ const and cast 'const Streams*' to non-const using
  const_cast<Stream *>() to match the IPU3 pipeline.
- Rename fitAnyPath() to fitsAllPaths().
- Expand documentation for why second stream is evaluated first if the
  fist stream can use either stream.
- Drop support for RGB888 and RGB656 for selfpath which was present in
  v2 as the driver delivers odd data when the frames are observed.

* Changes since v1
- Store formats in std::vector instead of std::array to avoid template
  usage for validate function.
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 210 +++++++++++++++++------
 1 file changed, 162 insertions(+), 48 deletions(-)

Hi Niklas,

On Fri, Sep 25, 2020 at 03:41:58AM +0200, Niklas Söderlund wrote:
> Extend the format validation to work with both main and self paths. The
> heuristics honors that the first stream in the configuration has the
> highest priority while still examining both streams for a best match.

I've heard you mentioning the priority of streams being defined by
their order multiple time, but this seems to be specific to this
pipeline handler, or have I missed it ?

>
> It is not possible to capture from the self path as the self stream is
> not yet exposed to applications.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
> * Changes since v2
> - Fix spelling in commit message.
> - Use Span<> instead of turning arrays to vectors.
> - Keep data_ const and cast 'const Streams*' to non-const using
>   const_cast<Stream *>() to match the IPU3 pipeline.
> - Rename fitAnyPath() to fitsAllPaths().
> - Expand documentation for why second stream is evaluated first if the
>   fist stream can use either stream.
> - Drop support for RGB888 and RGB656 for selfpath which was present in
>   v2 as the driver delivers odd data when the frames are observed.
>
> * Changes since v1
> - Store formats in std::vector instead of std::array to avoid template
>   usage for validate function.
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 210 +++++++++++++++++------
>  1 file changed, 162 insertions(+), 48 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index cd3049485746edd6..bd53183a034efaff 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -9,6 +9,7 @@
>  #include <array>
>  #include <iomanip>
>  #include <memory>
> +#include <numeric>
>  #include <queue>
>
>  #include <linux/media-bus-format.h>
> @@ -19,6 +20,7 @@
>  #include <libcamera/formats.h>
>  #include <libcamera/ipa/rkisp1.h>
>  #include <libcamera/request.h>
> +#include <libcamera/span.h>
>  #include <libcamera/stream.h>
>
>  #include "libcamera/internal/camera_sensor.h"
> @@ -50,6 +52,19 @@ constexpr std::array<PixelFormat, 7> RKISP1_RSZ_MP_FORMATS{
>  	formats::NV12,
>  	/* \todo Add support for 8-bit greyscale to DRM formats */
>  };
> +
> +constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };
> +constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 };

1920x1920 ? Maybe it's just the way it actually is

> +constexpr std::array<PixelFormat, 7> RKISP1_RSZ_SP_FORMATS{
> +	formats::YUYV,
> +	formats::YVYU,
> +	formats::VYUY,
> +	formats::NV16,
> +	formats::NV61,
> +	formats::NV21,
> +	formats::NV12,
> +	/* \todo Add support for BGR888 and RGB565 */
> +};

Aren't these equal to the main path ones ?

>  } /* namespace */
>
>  class PipelineHandlerRkISP1;
> @@ -181,6 +196,14 @@ public:
>  private:
>  	static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
>
> +	CameraConfiguration::Status validatePath(StreamConfiguration *cfg,
> +						 const Span<const PixelFormat> &formats,
> +						 const Size &min, const Size &max,
> +						 V4L2VideoDevice *video);
> +	CameraConfiguration::Status validateMainPath(StreamConfiguration *cfg);
> +	CameraConfiguration::Status validateSelfPath(StreamConfiguration *cfg);
> +	bool fitsAllPaths(const StreamConfiguration &cfg);
> +
>  	/*
>  	 * The RkISP1CameraData instance is guaranteed to be valid as long as the
>  	 * corresponding Camera instance is valid. In order to borrow a
> @@ -492,6 +515,69 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
>  	data_ = data;
>  }
>
> +CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(
> +	StreamConfiguration *cfg, const Span<const PixelFormat> &formats,
> +	const Size &min, const Size &max, V4L2VideoDevice *video)
> +{
> +	const StreamConfiguration reqCfg = *cfg;
> +	Status status = Valid;
> +
> +	if (std::find(formats.begin(), formats.end(), cfg->pixelFormat) ==
> +	    formats.end())
> +		cfg->pixelFormat = formats::NV12;
> +
> +	cfg->size.boundTo(max);
> +	cfg->size.expandTo(min);

This could be a one liner, but it's a matter of tastes

> +	cfg->bufferCount = RKISP1_BUFFER_COUNT;
> +
> +	V4L2DeviceFormat format = {};
> +	format.fourcc = video->toV4L2PixelFormat(cfg->pixelFormat);
> +	format.size = cfg->size;
> +
> +	int ret = video->tryFormat(&format);
> +	if (ret)
> +		return Invalid;
> +
> +	cfg->stride = format.planes[0].bpl;
> +	cfg->frameSize = format.planes[0].size;
> +
> +	if (cfg->pixelFormat != reqCfg.pixelFormat || cfg->size != reqCfg.size) {

Should you check for bufferCount too ?

> +		LOG(RkISP1, Debug)
> +			<< "Adjusting format from " << reqCfg.toString()
> +			<< " to " << cfg->toString();
> +		status = Adjusted;
> +	}
> +
> +	return status;
> +}
> +
> +CameraConfiguration::Status RkISP1CameraConfiguration::validateMainPath(StreamConfiguration *cfg)
> +{
> +	return validatePath(cfg, RKISP1_RSZ_MP_FORMATS, RKISP1_RSZ_MP_SRC_MIN,
> +			    RKISP1_RSZ_MP_SRC_MAX, data_->mainPathVideo_);
> +}
> +
> +CameraConfiguration::Status RkISP1CameraConfiguration::validateSelfPath(StreamConfiguration *cfg)
> +{
> +	return validatePath(cfg, RKISP1_RSZ_SP_FORMATS, RKISP1_RSZ_SP_SRC_MIN,
> +			    RKISP1_RSZ_SP_SRC_MAX, data_->selfPathVideo_);
> +}
> +
> +bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)
> +{
> +	StreamConfiguration config;
> +
> +	config = cfg;
> +	if (validateMainPath(&config) != Valid)

Shouldn't we accept Adjusted too if we want to check if the stream
'fits' the path ?

> +		return false;
> +
> +	config = cfg;
> +	if (validateSelfPath(&config) != Valid)
> +		return false;
> +
> +	return true;
> +}
> +
>  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  {
>  	const CameraSensor *sensor = data_->sensor_;
> @@ -501,22 +587,87 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  		return Invalid;
>
>  	/* Cap the number of entries to the available streams. */
> -	if (config_.size() > 1) {
> -		config_.resize(1);
> +	if (config_.size() > 2) {
> +		config_.resize(2);
>  		status = Adjusted;
>  	}
>
> -	StreamConfiguration &cfg = config_[0];
> -
> -	/* Adjust the pixel format. */
> -	if (std::find(RKISP1_RSZ_MP_FORMATS.begin(), RKISP1_RSZ_MP_FORMATS.end(),
> -		      cfg.pixelFormat) == RKISP1_RSZ_MP_FORMATS.end()) {
> -		LOG(RkISP1, Debug) << "Adjusting format to NV12";
> -		cfg.pixelFormat = formats::NV12,
> -		status = Adjusted;
> +	/*
> +	 * If there are more then one stream in the configuration figure out the
> +	 * order to evaluate the streams. The first stream has the highest
> +	 * priority but if both main path and self path can satisfy it evaluate
> +	 * second stream first as the first stream is guaranteed to work with
> +	 * whichever path is not used by the second one.
> +	 */
> +	std::vector<unsigned int> order(config_.size());
> +	std::iota(order.begin(), order.end(), 0);
> +	if (config_.size() == 2 && fitsAllPaths(config_[0]))
> +		std::reverse(order.begin(), order.end());

Let alone the implementation which is nice, is really the order of the
configurations that defines to which output they should be assigned ?
I'm not familiar with the platform, but in example, I would expect the
larger to go to the main path (as at this time they support the same
formats)

> +
> +	bool mainPathAvailable = true;
> +	bool selfPathAvailable = true;
> +	for (unsigned int index : order) {
> +		StreamConfiguration &cfg = config_[index];
> +
> +		/* Try to match stream without adjusting configuration. */
> +		if (mainPathAvailable) {
> +			StreamConfiguration tryCfg = cfg;
> +			if (validateMainPath(&tryCfg) == Valid) {
> +				mainPathAvailable = false;
> +				cfg = tryCfg;
> +				cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_));
> +				LOG(RkISP1, Debug) << "Exact match main";
> +				continue;
> +			}
> +		}
> +
> +		if (selfPathAvailable) {
> +			StreamConfiguration tryCfg = cfg;
> +			if (validateSelfPath(&tryCfg) == Valid) {
> +				selfPathAvailable = false;
> +				cfg = tryCfg;

Do you need to re-assign it if it's valid ?

> +				cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_));
> +				LOG(RkISP1, Debug) << "Exact match self";
> +				continue;
> +			}
> +		}
> +
> +		/* Try to match stream allowing adjusting configuration. */
> +		if (mainPathAvailable) {
> +			StreamConfiguration tryCfg = cfg;
> +			if (validateMainPath(&tryCfg) == Adjusted) {
> +				mainPathAvailable = false;
> +				cfg = tryCfg;
> +				cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_));
> +				status = Adjusted;
> +				LOG(RkISP1, Debug) << "Adjust match main";
> +				continue;
> +			}
> +		}
> +
> +		if (selfPathAvailable) {
> +			StreamConfiguration tryCfg = cfg;
> +			if (validateSelfPath(&tryCfg) == Adjusted) {
> +				selfPathAvailable = false;
> +				cfg = tryCfg;
> +				cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_));
> +				status = Adjusted;
> +				LOG(RkISP1, Debug) << "Adjust match self";

If I were to read this without reading the code, I would be puzzled.
same for other messages :)

> +				continue;
> +			}
> +		}

I dug out the comment from the review of the first version where I
asked "why" and the answer was that it's done not to adjust a stream
where it is not needed, so you initially only wants "valid" then try
accept "adjusted" if "valid" cannot be obtained.

So, what are the resons for "adjusting" ? Or either the pixel format
is wrong, and you would need to adjust on both nodes, or if the sizes
are larger the ones supported by the selfpath, in that case you
fallback to the main path and even if in that case they're too large,
adjust the stream. I think you could work this out easily if you sort
streams by size, but as I didn't get where "first as higher priority"
constraints come from, I might be mistaken.

If size sorted you try with main path first, if it's not taken and you
accept Valid || Adjusted. The second stream goes to the selfpath, and
you accept Valid || Adjusted there too without duplicating the check.

But if the priority order is justified or even part of the libcamera
API documentation, I think what you have here is fine.

Also, I don't know how RAW will work with this pipeline, but will this
double pass scale well in that case or there shouldn't be particular
issues ?

> +
> +		/* All paths rejected configuraiton. */
> +		LOG(RkISP1, Debug) << "Camera configuration not supported "
> +				   << cfg.toString();
> +		return Invalid;
>  	}
>
>  	/* Select the sensor format. */
> +	Size maxSize;
> +	for (const StreamConfiguration &cfg : config_)
> +		maxSize = std::max(maxSize, cfg.size);
> +
>  	sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,
>  					    MEDIA_BUS_FMT_SGBRG12_1X12,
>  					    MEDIA_BUS_FMT_SGRBG12_1X12,
> @@ -529,47 +680,10 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  					    MEDIA_BUS_FMT_SGBRG8_1X8,
>  					    MEDIA_BUS_FMT_SGRBG8_1X8,
>  					    MEDIA_BUS_FMT_SRGGB8_1X8 },
> -					  cfg.size);
> +					  maxSize);
>  	if (sensorFormat_.size.isNull())
>  		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.isNull()) {
> -		cfg.size.width = 1280;
> -		cfg.size.height = 1280 * sensorFormat_.size.height
> -				/ sensorFormat_.size.width;
> -	}
> -
> -	cfg.size.boundTo(RKISP1_RSZ_MP_SRC_MAX);
> -	cfg.size.expandTo(RKISP1_RSZ_MP_SRC_MIN);
> -
> -	if (cfg.size != size) {
> -		LOG(RkISP1, Debug)
> -			<< "Adjusting size from " << size.toString()
> -			<< " to " << cfg.size.toString();
> -		status = Adjusted;
> -	}
> -
> -	cfg.bufferCount = RKISP1_BUFFER_COUNT;
> -
> -	V4L2DeviceFormat format = {};
> -	format.fourcc = data_->mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat);
> -	format.size = cfg.size;
> -
> -	int ret = data_->mainPathVideo_->tryFormat(&format);
> -	if (ret)
> -		return Invalid;
> -
> -	cfg.stride = format.planes[0].bpl;
> -	cfg.frameSize = format.planes[0].size;
> -
>  	return status;
>  }
>
> --
> 2.28.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

Thanks for your feedback.

On 2020-09-25 17:02:45 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Fri, Sep 25, 2020 at 03:41:58AM +0200, Niklas Söderlund wrote:
> > Extend the format validation to work with both main and self paths. The
> > heuristics honors that the first stream in the configuration has the
> > highest priority while still examining both streams for a best match.
> 
> I've heard you mentioning the priority of streams being defined by
> their order multiple time, but this seems to be specific to this
> pipeline handler, or have I missed it ?

Well the first role in the array given to generateConfiguration() have 
higher priority then the second. So this laves that the first element in 
the returned configuration shall have higher priority then the second 
right?

> 
> >
> > It is not possible to capture from the self path as the self stream is
> > not yet exposed to applications.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> > * Changes since v2
> > - Fix spelling in commit message.
> > - Use Span<> instead of turning arrays to vectors.
> > - Keep data_ const and cast 'const Streams*' to non-const using
> >   const_cast<Stream *>() to match the IPU3 pipeline.
> > - Rename fitAnyPath() to fitsAllPaths().
> > - Expand documentation for why second stream is evaluated first if the
> >   fist stream can use either stream.
> > - Drop support for RGB888 and RGB656 for selfpath which was present in
> >   v2 as the driver delivers odd data when the frames are observed.
> >
> > * Changes since v1
> > - Store formats in std::vector instead of std::array to avoid template
> >   usage for validate function.
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 210 +++++++++++++++++------
> >  1 file changed, 162 insertions(+), 48 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index cd3049485746edd6..bd53183a034efaff 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -9,6 +9,7 @@
> >  #include <array>
> >  #include <iomanip>
> >  #include <memory>
> > +#include <numeric>
> >  #include <queue>
> >
> >  #include <linux/media-bus-format.h>
> > @@ -19,6 +20,7 @@
> >  #include <libcamera/formats.h>
> >  #include <libcamera/ipa/rkisp1.h>
> >  #include <libcamera/request.h>
> > +#include <libcamera/span.h>
> >  #include <libcamera/stream.h>
> >
> >  #include "libcamera/internal/camera_sensor.h"
> > @@ -50,6 +52,19 @@ constexpr std::array<PixelFormat, 7> RKISP1_RSZ_MP_FORMATS{
> >  	formats::NV12,
> >  	/* \todo Add support for 8-bit greyscale to DRM formats */
> >  };
> > +
> > +constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };
> > +constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 };
> 
> 1920x1920 ? Maybe it's just the way it actually is

At least in the kernel sources.

> 
> > +constexpr std::array<PixelFormat, 7> RKISP1_RSZ_SP_FORMATS{
> > +	formats::YUYV,
> > +	formats::YVYU,
> > +	formats::VYUY,
> > +	formats::NV16,
> > +	formats::NV61,
> > +	formats::NV21,
> > +	formats::NV12,
> > +	/* \todo Add support for BGR888 and RGB565 */
> > +};
> 
> Aren't these equal to the main path ones ?

Yes and no :-) The enabled ones are, but the todos are not.

> 
> >  } /* namespace */
> >
> >  class PipelineHandlerRkISP1;
> > @@ -181,6 +196,14 @@ public:
> >  private:
> >  	static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
> >
> > +	CameraConfiguration::Status validatePath(StreamConfiguration *cfg,
> > +						 const Span<const PixelFormat> &formats,
> > +						 const Size &min, const Size &max,
> > +						 V4L2VideoDevice *video);
> > +	CameraConfiguration::Status validateMainPath(StreamConfiguration *cfg);
> > +	CameraConfiguration::Status validateSelfPath(StreamConfiguration *cfg);
> > +	bool fitsAllPaths(const StreamConfiguration &cfg);
> > +
> >  	/*
> >  	 * The RkISP1CameraData instance is guaranteed to be valid as long as the
> >  	 * corresponding Camera instance is valid. In order to borrow a
> > @@ -492,6 +515,69 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
> >  	data_ = data;
> >  }
> >
> > +CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(
> > +	StreamConfiguration *cfg, const Span<const PixelFormat> &formats,
> > +	const Size &min, const Size &max, V4L2VideoDevice *video)
> > +{
> > +	const StreamConfiguration reqCfg = *cfg;
> > +	Status status = Valid;
> > +
> > +	if (std::find(formats.begin(), formats.end(), cfg->pixelFormat) ==
> > +	    formats.end())
> > +		cfg->pixelFormat = formats::NV12;
> > +
> > +	cfg->size.boundTo(max);
> > +	cfg->size.expandTo(min);
> 
> This could be a one liner, but it's a matter of tastes

As in prev version I like this much better then to group them in on 
line.  But as you say it's matter of taste.

> 
> > +	cfg->bufferCount = RKISP1_BUFFER_COUNT;
> > +
> > +	V4L2DeviceFormat format = {};
> > +	format.fourcc = video->toV4L2PixelFormat(cfg->pixelFormat);
> > +	format.size = cfg->size;
> > +
> > +	int ret = video->tryFormat(&format);
> > +	if (ret)
> > +		return Invalid;
> > +
> > +	cfg->stride = format.planes[0].bpl;
> > +	cfg->frameSize = format.planes[0].size;
> > +
> > +	if (cfg->pixelFormat != reqCfg.pixelFormat || cfg->size != reqCfg.size) {
> 
> Should you check for bufferCount too ?

I don't think so, similar to that we don't check stride and frameSize 
right?

> 
> > +		LOG(RkISP1, Debug)
> > +			<< "Adjusting format from " << reqCfg.toString()
> > +			<< " to " << cfg->toString();
> > +		status = Adjusted;
> > +	}
> > +
> > +	return status;
> > +}
> > +
> > +CameraConfiguration::Status RkISP1CameraConfiguration::validateMainPath(StreamConfiguration *cfg)
> > +{
> > +	return validatePath(cfg, RKISP1_RSZ_MP_FORMATS, RKISP1_RSZ_MP_SRC_MIN,
> > +			    RKISP1_RSZ_MP_SRC_MAX, data_->mainPathVideo_);
> > +}
> > +
> > +CameraConfiguration::Status RkISP1CameraConfiguration::validateSelfPath(StreamConfiguration *cfg)
> > +{
> > +	return validatePath(cfg, RKISP1_RSZ_SP_FORMATS, RKISP1_RSZ_SP_SRC_MIN,
> > +			    RKISP1_RSZ_SP_SRC_MAX, data_->selfPathVideo_);
> > +}
> > +
> > +bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)
> > +{
> > +	StreamConfiguration config;
> > +
> > +	config = cfg;
> > +	if (validateMainPath(&config) != Valid)
> 
> Shouldn't we accept Adjusted too if we want to check if the stream
> 'fits' the path ?

No.

We wish to check for exact match of the requested format. What we wish 
to learn is if the requested configuration can be satisfied on both 
paths without being adjusted.

> 
> > +		return false;
> > +
> > +	config = cfg;
> > +	if (validateSelfPath(&config) != Valid)
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> >  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> >  {
> >  	const CameraSensor *sensor = data_->sensor_;
> > @@ -501,22 +587,87 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> >  		return Invalid;
> >
> >  	/* Cap the number of entries to the available streams. */
> > -	if (config_.size() > 1) {
> > -		config_.resize(1);
> > +	if (config_.size() > 2) {
> > +		config_.resize(2);
> >  		status = Adjusted;
> >  	}
> >
> > -	StreamConfiguration &cfg = config_[0];
> > -
> > -	/* Adjust the pixel format. */
> > -	if (std::find(RKISP1_RSZ_MP_FORMATS.begin(), RKISP1_RSZ_MP_FORMATS.end(),
> > -		      cfg.pixelFormat) == RKISP1_RSZ_MP_FORMATS.end()) {
> > -		LOG(RkISP1, Debug) << "Adjusting format to NV12";
> > -		cfg.pixelFormat = formats::NV12,
> > -		status = Adjusted;
> > +	/*
> > +	 * If there are more then one stream in the configuration figure out the
> > +	 * order to evaluate the streams. The first stream has the highest
> > +	 * priority but if both main path and self path can satisfy it evaluate
> > +	 * second stream first as the first stream is guaranteed to work with
> > +	 * whichever path is not used by the second one.
> > +	 */
> > +	std::vector<unsigned int> order(config_.size());
> > +	std::iota(order.begin(), order.end(), 0);
> > +	if (config_.size() == 2 && fitsAllPaths(config_[0]))
> > +		std::reverse(order.begin(), order.end());
> 
> Let alone the implementation which is nice, is really the order of the
> configurations that defines to which output they should be assigned ?
> I'm not familiar with the platform, but in example, I would expect the
> larger to go to the main path (as at this time they support the same
> formats)

No this logic tries to maximize the possibility for both streams to be 
fully satisfied while still respecting that the first configuration have 
higher priority if not both can be satisfied.

The for-loop bellows assign hardware in a first come first serve fashion 
and once a path have been assigned the assignment is never re-evaluated.  
It first tries to see if the configuration can be an exact match on any 
of the paths and then if it can be adjusted to fit (that is why two 
passes are needed).

Above we check with fitsAllPaths(config_[0]) if the first configuration 
(with has the highest priority) can fit on any path without being 
adjusted. If so then we know it's safe to swap the order and of the 
configurations as to maximize config_[1] to find an exact match on one 
of the pats. We can do this as we already know config_[0] will have en 
exact match on either of them.

If config_[0] can not find an exact match on both paths we do our best 
to make it fit first and config_[1] are stick with what ever resources 
are left.

> 
> > +
> > +	bool mainPathAvailable = true;
> > +	bool selfPathAvailable = true;
> > +	for (unsigned int index : order) {
> > +		StreamConfiguration &cfg = config_[index];
> > +
> > +		/* Try to match stream without adjusting configuration. */
> > +		if (mainPathAvailable) {
> > +			StreamConfiguration tryCfg = cfg;
> > +			if (validateMainPath(&tryCfg) == Valid) {
> > +				mainPathAvailable = false;
> > +				cfg = tryCfg;
> > +				cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_));
> > +				LOG(RkISP1, Debug) << "Exact match main";
> > +				continue;
> > +			}
> > +		}
> > +
> > +		if (selfPathAvailable) {
> > +			StreamConfiguration tryCfg = cfg;
> > +			if (validateSelfPath(&tryCfg) == Valid) {
> > +				selfPathAvailable = false;
> > +				cfg = tryCfg;
> 
> Do you need to re-assign it if it's valid ?

Good point, no it's not needed.

> 
> > +				cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_));
> > +				LOG(RkISP1, Debug) << "Exact match self";
> > +				continue;
> > +			}
> > +		}
> > +
> > +		/* Try to match stream allowing adjusting configuration. */
> > +		if (mainPathAvailable) {
> > +			StreamConfiguration tryCfg = cfg;
> > +			if (validateMainPath(&tryCfg) == Adjusted) {
> > +				mainPathAvailable = false;
> > +				cfg = tryCfg;
> > +				cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_));
> > +				status = Adjusted;
> > +				LOG(RkISP1, Debug) << "Adjust match main";
> > +				continue;
> > +			}
> > +		}
> > +
> > +		if (selfPathAvailable) {
> > +			StreamConfiguration tryCfg = cfg;
> > +			if (validateSelfPath(&tryCfg) == Adjusted) {
> > +				selfPathAvailable = false;
> > +				cfg = tryCfg;
> > +				cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_));
> > +				status = Adjusted;
> > +				LOG(RkISP1, Debug) << "Adjust match self";
> 
> If I were to read this without reading the code, I would be puzzled.
> same for other messages :)

I will drop them as they seem to confuse more then help.

> 
> > +				continue;
> > +			}
> > +		}
> 
> I dug out the comment from the review of the first version where I
> asked "why" and the answer was that it's done not to adjust a stream
> where it is not needed, so you initially only wants "valid" then try
> accept "adjusted" if "valid" cannot be obtained.
> 
> So, what are the resons for "adjusting" ? Or either the pixel format
> is wrong, and you would need to adjust on both nodes, or if the sizes
> are larger the ones supported by the selfpath, in that case you
> fallback to the main path and even if in that case they're too large,
> adjust the stream. I think you could work this out easily if you sort
> streams by size, but as I didn't get where "first as higher priority"
> constraints come from, I might be mistaken.
> 
> If size sorted you try with main path first, if it's not taken and you
> accept Valid || Adjusted. The second stream goes to the selfpath, and
> you accept Valid || Adjusted there too without duplicating the check.
> 
> But if the priority order is justified or even part of the libcamera
> API documentation, I think what you have here is fine.

If it was only sizes that differed between the two yes, but it's not.  
The main path can support RAW formats while the self path can support 
RGB. So sorting by size is unfortunately not possible as we also need to 
consider the formats.

> 
> Also, I don't know how RAW will work with this pipeline, but will this
> double pass scale well in that case or there shouldn't be particular
> issues ?

RAW will be a special case on this pipeline as it will limit it to one 
stream. When capturing RAW only the main path can be active.

But same question for RGB formats which are only supported by the self 
path. No this is tested with that in mind as adding BGR888 to the list 
of supported formats in RKISP1_RSZ_SP_FORMATS is all that is needed to 
enable it to be captured. But the frames looks a bit wierd in qcam so I 
will investigate this on top of this series but I do not wish to block 
this already too large series on also enabeling support for BGR888 and 
RGB656.

> 
> > +
> > +		/* All paths rejected configuraiton. */
> > +		LOG(RkISP1, Debug) << "Camera configuration not supported "
> > +				   << cfg.toString();
> > +		return Invalid;
> >  	}
> >
> >  	/* Select the sensor format. */
> > +	Size maxSize;
> > +	for (const StreamConfiguration &cfg : config_)
> > +		maxSize = std::max(maxSize, cfg.size);
> > +
> >  	sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,
> >  					    MEDIA_BUS_FMT_SGBRG12_1X12,
> >  					    MEDIA_BUS_FMT_SGRBG12_1X12,
> > @@ -529,47 +680,10 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> >  					    MEDIA_BUS_FMT_SGBRG8_1X8,
> >  					    MEDIA_BUS_FMT_SGRBG8_1X8,
> >  					    MEDIA_BUS_FMT_SRGGB8_1X8 },
> > -					  cfg.size);
> > +					  maxSize);
> >  	if (sensorFormat_.size.isNull())
> >  		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.isNull()) {
> > -		cfg.size.width = 1280;
> > -		cfg.size.height = 1280 * sensorFormat_.size.height
> > -				/ sensorFormat_.size.width;
> > -	}
> > -
> > -	cfg.size.boundTo(RKISP1_RSZ_MP_SRC_MAX);
> > -	cfg.size.expandTo(RKISP1_RSZ_MP_SRC_MIN);
> > -
> > -	if (cfg.size != size) {
> > -		LOG(RkISP1, Debug)
> > -			<< "Adjusting size from " << size.toString()
> > -			<< " to " << cfg.size.toString();
> > -		status = Adjusted;
> > -	}
> > -
> > -	cfg.bufferCount = RKISP1_BUFFER_COUNT;
> > -
> > -	V4L2DeviceFormat format = {};
> > -	format.fourcc = data_->mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat);
> > -	format.size = cfg.size;
> > -
> > -	int ret = data_->mainPathVideo_->tryFormat(&format);
> > -	if (ret)
> > -		return Invalid;
> > -
> > -	cfg.stride = format.planes[0].bpl;
> > -	cfg.frameSize = format.planes[0].size;
> > -
> >  	return status;
> >  }
> >
> > --
> > 2.28.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

Hi Niklas,

On Fri, Sep 25, 2020 at 06:48:25PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your feedback.
>
> On 2020-09-25 17:02:45 +0200, Jacopo Mondi wrote:
> > Hi Niklas,
> >
> > On Fri, Sep 25, 2020 at 03:41:58AM +0200, Niklas Söderlund wrote:
> > > Extend the format validation to work with both main and self paths. The
> > > heuristics honors that the first stream in the configuration has the
> > > highest priority while still examining both streams for a best match.
> >
> > I've heard you mentioning the priority of streams being defined by
> > their order multiple time, but this seems to be specific to this
> > pipeline handler, or have I missed it ?
>
> Well the first role in the array given to generateConfiguration() have
> higher priority then the second. So this laves that the first element in
> the returned configuration shall have higher priority then the second
> right?
>

My point, in this patch and other, is that I don't see where the
priority order constraint comes from. I don't see it in the
Camera::generateConfiguration() or Camera::configure() documentation
and I'm wondering if that's a pipeline-specific behaviour (which, in
general, we should try to introduce, imho).

Thanks
  j

> >
> > >
> > > It is not possible to capture from the self path as the self stream is
> > > not yet exposed to applications.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > ---
> > > * Changes since v2
> > > - Fix spelling in commit message.
> > > - Use Span<> instead of turning arrays to vectors.
> > > - Keep data_ const and cast 'const Streams*' to non-const using
> > >   const_cast<Stream *>() to match the IPU3 pipeline.
> > > - Rename fitAnyPath() to fitsAllPaths().
> > > - Expand documentation for why second stream is evaluated first if the
> > >   fist stream can use either stream.
> > > - Drop support for RGB888 and RGB656 for selfpath which was present in
> > >   v2 as the driver delivers odd data when the frames are observed.
> > >
> > > * Changes since v1
> > > - Store formats in std::vector instead of std::array to avoid template
> > >   usage for validate function.
> > > ---
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 210 +++++++++++++++++------
> > >  1 file changed, 162 insertions(+), 48 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index cd3049485746edd6..bd53183a034efaff 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -9,6 +9,7 @@
> > >  #include <array>
> > >  #include <iomanip>
> > >  #include <memory>
> > > +#include <numeric>
> > >  #include <queue>
> > >
> > >  #include <linux/media-bus-format.h>
> > > @@ -19,6 +20,7 @@
> > >  #include <libcamera/formats.h>
> > >  #include <libcamera/ipa/rkisp1.h>
> > >  #include <libcamera/request.h>
> > > +#include <libcamera/span.h>
> > >  #include <libcamera/stream.h>
> > >
> > >  #include "libcamera/internal/camera_sensor.h"
> > > @@ -50,6 +52,19 @@ constexpr std::array<PixelFormat, 7> RKISP1_RSZ_MP_FORMATS{
> > >  	formats::NV12,
> > >  	/* \todo Add support for 8-bit greyscale to DRM formats */
> > >  };
> > > +
> > > +constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };
> > > +constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 };
> >
> > 1920x1920 ? Maybe it's just the way it actually is
>
> At least in the kernel sources.
>
> >
> > > +constexpr std::array<PixelFormat, 7> RKISP1_RSZ_SP_FORMATS{
> > > +	formats::YUYV,
> > > +	formats::YVYU,
> > > +	formats::VYUY,
> > > +	formats::NV16,
> > > +	formats::NV61,
> > > +	formats::NV21,
> > > +	formats::NV12,
> > > +	/* \todo Add support for BGR888 and RGB565 */
> > > +};
> >
> > Aren't these equal to the main path ones ?
>
> Yes and no :-) The enabled ones are, but the todos are not.
>
> >
> > >  } /* namespace */
> > >
> > >  class PipelineHandlerRkISP1;
> > > @@ -181,6 +196,14 @@ public:
> > >  private:
> > >  	static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
> > >
> > > +	CameraConfiguration::Status validatePath(StreamConfiguration *cfg,
> > > +						 const Span<const PixelFormat> &formats,
> > > +						 const Size &min, const Size &max,
> > > +						 V4L2VideoDevice *video);
> > > +	CameraConfiguration::Status validateMainPath(StreamConfiguration *cfg);
> > > +	CameraConfiguration::Status validateSelfPath(StreamConfiguration *cfg);
> > > +	bool fitsAllPaths(const StreamConfiguration &cfg);
> > > +
> > >  	/*
> > >  	 * The RkISP1CameraData instance is guaranteed to be valid as long as the
> > >  	 * corresponding Camera instance is valid. In order to borrow a
> > > @@ -492,6 +515,69 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
> > >  	data_ = data;
> > >  }
> > >
> > > +CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(
> > > +	StreamConfiguration *cfg, const Span<const PixelFormat> &formats,
> > > +	const Size &min, const Size &max, V4L2VideoDevice *video)
> > > +{
> > > +	const StreamConfiguration reqCfg = *cfg;
> > > +	Status status = Valid;
> > > +
> > > +	if (std::find(formats.begin(), formats.end(), cfg->pixelFormat) ==
> > > +	    formats.end())
> > > +		cfg->pixelFormat = formats::NV12;
> > > +
> > > +	cfg->size.boundTo(max);
> > > +	cfg->size.expandTo(min);
> >
> > This could be a one liner, but it's a matter of tastes
>
> As in prev version I like this much better then to group them in on
> line.  But as you say it's matter of taste.
>
> >
> > > +	cfg->bufferCount = RKISP1_BUFFER_COUNT;
> > > +
> > > +	V4L2DeviceFormat format = {};
> > > +	format.fourcc = video->toV4L2PixelFormat(cfg->pixelFormat);
> > > +	format.size = cfg->size;
> > > +
> > > +	int ret = video->tryFormat(&format);
> > > +	if (ret)
> > > +		return Invalid;
> > > +
> > > +	cfg->stride = format.planes[0].bpl;
> > > +	cfg->frameSize = format.planes[0].size;
> > > +
> > > +	if (cfg->pixelFormat != reqCfg.pixelFormat || cfg->size != reqCfg.size) {
> >
> > Should you check for bufferCount too ?
>
> I don't think so, similar to that we don't check stride and frameSize
> right?
>

not sure you know... stride and frameSize are output information,
bufferCount can actually be set by the application, doesn't it ?

> >
> > > +		LOG(RkISP1, Debug)
> > > +			<< "Adjusting format from " << reqCfg.toString()
> > > +			<< " to " << cfg->toString();
> > > +		status = Adjusted;
> > > +	}
> > > +
> > > +	return status;
> > > +}
> > > +
> > > +CameraConfiguration::Status RkISP1CameraConfiguration::validateMainPath(StreamConfiguration *cfg)
> > > +{
> > > +	return validatePath(cfg, RKISP1_RSZ_MP_FORMATS, RKISP1_RSZ_MP_SRC_MIN,
> > > +			    RKISP1_RSZ_MP_SRC_MAX, data_->mainPathVideo_);
> > > +}
> > > +
> > > +CameraConfiguration::Status RkISP1CameraConfiguration::validateSelfPath(StreamConfiguration *cfg)
> > > +{
> > > +	return validatePath(cfg, RKISP1_RSZ_SP_FORMATS, RKISP1_RSZ_SP_SRC_MIN,
> > > +			    RKISP1_RSZ_SP_SRC_MAX, data_->selfPathVideo_);
> > > +}
> > > +
> > > +bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)
> > > +{
> > > +	StreamConfiguration config;
> > > +
> > > +	config = cfg;
> > > +	if (validateMainPath(&config) != Valid)
> >
> > Shouldn't we accept Adjusted too if we want to check if the stream
> > 'fits' the path ?
>
> No.
>
> We wish to check for exact match of the requested format. What we wish
> to learn is if the requested configuration can be satisfied on both
> paths without being adjusted.
>
> >
> > > +		return false;
> > > +
> > > +	config = cfg;
> > > +	if (validateSelfPath(&config) != Valid)
> > > +		return false;
> > > +
> > > +	return true;
> > > +}
> > > +
> > >  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> > >  {
> > >  	const CameraSensor *sensor = data_->sensor_;
> > > @@ -501,22 +587,87 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> > >  		return Invalid;
> > >
> > >  	/* Cap the number of entries to the available streams. */
> > > -	if (config_.size() > 1) {
> > > -		config_.resize(1);
> > > +	if (config_.size() > 2) {
> > > +		config_.resize(2);
> > >  		status = Adjusted;
> > >  	}
> > >
> > > -	StreamConfiguration &cfg = config_[0];
> > > -
> > > -	/* Adjust the pixel format. */
> > > -	if (std::find(RKISP1_RSZ_MP_FORMATS.begin(), RKISP1_RSZ_MP_FORMATS.end(),
> > > -		      cfg.pixelFormat) == RKISP1_RSZ_MP_FORMATS.end()) {
> > > -		LOG(RkISP1, Debug) << "Adjusting format to NV12";
> > > -		cfg.pixelFormat = formats::NV12,
> > > -		status = Adjusted;
> > > +	/*
> > > +	 * If there are more then one stream in the configuration figure out the
> > > +	 * order to evaluate the streams. The first stream has the highest
> > > +	 * priority but if both main path and self path can satisfy it evaluate
> > > +	 * second stream first as the first stream is guaranteed to work with
> > > +	 * whichever path is not used by the second one.
> > > +	 */
> > > +	std::vector<unsigned int> order(config_.size());
> > > +	std::iota(order.begin(), order.end(), 0);
> > > +	if (config_.size() == 2 && fitsAllPaths(config_[0]))
> > > +		std::reverse(order.begin(), order.end());
> >
> > Let alone the implementation which is nice, is really the order of the
> > configurations that defines to which output they should be assigned ?

Asking the same question again, I might have missed where the priority
assignment requirement comes from.


> > I'm not familiar with the platform, but in example, I would expect the
> > larger to go to the main path (as at this time they support the same
> > formats)
>
> No this logic tries to maximize the possibility for both streams to be
> fully satisfied while still respecting that the first configuration have
> higher priority if not both can be satisfied.
>
> The for-loop bellows assign hardware in a first come first serve fashion
> and once a path have been assigned the assignment is never re-evaluated.
> It first tries to see if the configuration can be an exact match on any
> of the paths and then if it can be adjusted to fit (that is why two
> passes are needed).
>
> Above we check with fitsAllPaths(config_[0]) if the first configuration
> (with has the highest priority) can fit on any path without being
> adjusted. If so then we know it's safe to swap the order and of the
> configurations as to maximize config_[1] to find an exact match on one
> of the pats. We can do this as we already know config_[0] will have en
> exact match on either of them.
>
> If config_[0] can not find an exact match on both paths we do our best
> to make it fit first and config_[1] are stick with what ever resources
> are left.
>
> >
> > > +
> > > +	bool mainPathAvailable = true;
> > > +	bool selfPathAvailable = true;
> > > +	for (unsigned int index : order) {
> > > +		StreamConfiguration &cfg = config_[index];
> > > +
> > > +		/* Try to match stream without adjusting configuration. */
> > > +		if (mainPathAvailable) {
> > > +			StreamConfiguration tryCfg = cfg;
> > > +			if (validateMainPath(&tryCfg) == Valid) {
> > > +				mainPathAvailable = false;
> > > +				cfg = tryCfg;
> > > +				cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_));
> > > +				LOG(RkISP1, Debug) << "Exact match main";
> > > +				continue;
> > > +			}
> > > +		}
> > > +
> > > +		if (selfPathAvailable) {
> > > +			StreamConfiguration tryCfg = cfg;
> > > +			if (validateSelfPath(&tryCfg) == Valid) {
> > > +				selfPathAvailable = false;
> > > +				cfg = tryCfg;
> >
> > Do you need to re-assign it if it's valid ?
>
> Good point, no it's not needed.
>

Wait, maybe bufferCount or stride has changed, as their modification
doesn't 'Adjust' the stream status.

> >
> > > +				cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_));
> > > +				LOG(RkISP1, Debug) << "Exact match self";
> > > +				continue;
> > > +			}
> > > +		}
> > > +
> > > +		/* Try to match stream allowing adjusting configuration. */
> > > +		if (mainPathAvailable) {
> > > +			StreamConfiguration tryCfg = cfg;
> > > +			if (validateMainPath(&tryCfg) == Adjusted) {
> > > +				mainPathAvailable = false;
> > > +				cfg = tryCfg;
> > > +				cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_));
> > > +				status = Adjusted;
> > > +				LOG(RkISP1, Debug) << "Adjust match main";
> > > +				continue;
> > > +			}
> > > +		}
> > > +
> > > +		if (selfPathAvailable) {
> > > +			StreamConfiguration tryCfg = cfg;
> > > +			if (validateSelfPath(&tryCfg) == Adjusted) {
> > > +				selfPathAvailable = false;
> > > +				cfg = tryCfg;
> > > +				cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_));
> > > +				status = Adjusted;
> > > +				LOG(RkISP1, Debug) << "Adjust match self";
> >
> > If I were to read this without reading the code, I would be puzzled.
> > same for other messages :)
>
> I will drop them as they seem to confuse more then help.
>

Thanks, I think it could help if expanded. As a reference on IPU3:

        LOG(IPU3, Debug) << "Assigned " << cfg->toString()
                         << " to the main output";

> >
> > > +				continue;
> > > +			}
> > > +		}
> >
> > I dug out the comment from the review of the first version where I
> > asked "why" and the answer was that it's done not to adjust a stream
> > where it is not needed, so you initially only wants "valid" then try
> > accept "adjusted" if "valid" cannot be obtained.
> >
> > So, what are the resons for "adjusting" ? Or either the pixel format
> > is wrong, and you would need to adjust on both nodes, or if the sizes
> > are larger the ones supported by the selfpath, in that case you
> > fallback to the main path and even if in that case they're too large,
> > adjust the stream. I think you could work this out easily if you sort
> > streams by size, but as I didn't get where "first as higher priority"
> > constraints come from, I might be mistaken.
> >
> > If size sorted you try with main path first, if it's not taken and you
> > accept Valid || Adjusted. The second stream goes to the selfpath, and
> > you accept Valid || Adjusted there too without duplicating the check.
> >
> > But if the priority order is justified or even part of the libcamera
> > API documentation, I think what you have here is fine.
>
> If it was only sizes that differed between the two yes, but it's not.
> The main path can support RAW formats while the self path can support
> RGB. So sorting by size is unfortunately not possible as we also need to
> consider the formats.

But in that case assigning by format is even easier. RAW goes to main
and RGB goes to self.

>
> >
> > Also, I don't know how RAW will work with this pipeline, but will this
> > double pass scale well in that case or there shouldn't be particular
> > issues ?
>
> RAW will be a special case on this pipeline as it will limit it to one
> stream. When capturing RAW only the main path can be active.
>
> But same question for RGB formats which are only supported by the self
> path. No this is tested with that in mind as adding BGR888 to the list
> of supported formats in RKISP1_RSZ_SP_FORMATS is all that is needed to
> enable it to be captured. But the frames looks a bit wierd in qcam so I
> will investigate this on top of this series but I do not wish to block
> this already too large series on also enabeling support for BGR888 and
> RGB656.

Agreed on non blocking to wait for RGB.

>
> >
> > > +
> > > +		/* All paths rejected configuraiton. */
> > > +		LOG(RkISP1, Debug) << "Camera configuration not supported "
> > > +				   << cfg.toString();
> > > +		return Invalid;
> > >  	}
> > >
> > >  	/* Select the sensor format. */
> > > +	Size maxSize;
> > > +	for (const StreamConfiguration &cfg : config_)
> > > +		maxSize = std::max(maxSize, cfg.size);
> > > +
> > >  	sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,
> > >  					    MEDIA_BUS_FMT_SGBRG12_1X12,
> > >  					    MEDIA_BUS_FMT_SGRBG12_1X12,
> > > @@ -529,47 +680,10 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> > >  					    MEDIA_BUS_FMT_SGBRG8_1X8,
> > >  					    MEDIA_BUS_FMT_SGRBG8_1X8,
> > >  					    MEDIA_BUS_FMT_SRGGB8_1X8 },
> > > -					  cfg.size);
> > > +					  maxSize);
> > >  	if (sensorFormat_.size.isNull())
> > >  		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.isNull()) {
> > > -		cfg.size.width = 1280;
> > > -		cfg.size.height = 1280 * sensorFormat_.size.height
> > > -				/ sensorFormat_.size.width;
> > > -	}
> > > -
> > > -	cfg.size.boundTo(RKISP1_RSZ_MP_SRC_MAX);
> > > -	cfg.size.expandTo(RKISP1_RSZ_MP_SRC_MIN);
> > > -
> > > -	if (cfg.size != size) {
> > > -		LOG(RkISP1, Debug)
> > > -			<< "Adjusting size from " << size.toString()
> > > -			<< " to " << cfg.size.toString();
> > > -		status = Adjusted;
> > > -	}
> > > -
> > > -	cfg.bufferCount = RKISP1_BUFFER_COUNT;
> > > -
> > > -	V4L2DeviceFormat format = {};
> > > -	format.fourcc = data_->mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat);
> > > -	format.size = cfg.size;
> > > -
> > > -	int ret = data_->mainPathVideo_->tryFormat(&format);
> > > -	if (ret)
> > > -		return Invalid;
> > > -
> > > -	cfg.stride = format.planes[0].bpl;
> > > -	cfg.frameSize = format.planes[0].size;
> > > -
> > >  	return status;
> > >  }
> > >
> > > --
> > > 2.28.0
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund

Hi Jacopo,

On Mon, Sep 28, 2020 at 10:34:49AM +0200, Jacopo Mondi wrote:
> On Fri, Sep 25, 2020 at 06:48:25PM +0200, Niklas Söderlund wrote:
> > On 2020-09-25 17:02:45 +0200, Jacopo Mondi wrote:
> > > On Fri, Sep 25, 2020 at 03:41:58AM +0200, Niklas Söderlund wrote:
> > > > Extend the format validation to work with both main and self paths. The
> > > > heuristics honors that the first stream in the configuration has the
> > > > highest priority while still examining both streams for a best match.
> > >
> > > I've heard you mentioning the priority of streams being defined by
> > > their order multiple time, but this seems to be specific to this
> > > pipeline handler, or have I missed it ?
> >
> > Well the first role in the array given to generateConfiguration() have
> > higher priority then the second. So this laves that the first element in
> > the returned configuration shall have higher priority then the second
> > right?
> 
> My point, in this patch and other, is that I don't see where the
> priority order constraint comes from. I don't see it in the
> Camera::generateConfiguration() or Camera::configure() documentation
> and I'm wondering if that's a pipeline-specific behaviour (which, in
> general, we should try to introduce, imho).

I would have sworn this was documented, but it seems not to be the case.
I consider this as a standard behaviour, not API specific. I'll make
sure to document priorities in the configuration API rework.

> > > > It is not possible to capture from the self path as the self stream is
> > > > not yet exposed to applications.
> > > >
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > > ---
> > > > * Changes since v2
> > > > - Fix spelling in commit message.
> > > > - Use Span<> instead of turning arrays to vectors.
> > > > - Keep data_ const and cast 'const Streams*' to non-const using
> > > >   const_cast<Stream *>() to match the IPU3 pipeline.
> > > > - Rename fitAnyPath() to fitsAllPaths().
> > > > - Expand documentation for why second stream is evaluated first if the
> > > >   fist stream can use either stream.
> > > > - Drop support for RGB888 and RGB656 for selfpath which was present in
> > > >   v2 as the driver delivers odd data when the frames are observed.
> > > >
> > > > * Changes since v1
> > > > - Store formats in std::vector instead of std::array to avoid template
> > > >   usage for validate function.
> > > > ---
> > > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 210 +++++++++++++++++------
> > > >  1 file changed, 162 insertions(+), 48 deletions(-)
> > > >
> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > index cd3049485746edd6..bd53183a034efaff 100644
> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > @@ -9,6 +9,7 @@
> > > >  #include <array>
> > > >  #include <iomanip>
> > > >  #include <memory>
> > > > +#include <numeric>
> > > >  #include <queue>
> > > >
> > > >  #include <linux/media-bus-format.h>
> > > > @@ -19,6 +20,7 @@
> > > >  #include <libcamera/formats.h>
> > > >  #include <libcamera/ipa/rkisp1.h>
> > > >  #include <libcamera/request.h>
> > > > +#include <libcamera/span.h>
> > > >  #include <libcamera/stream.h>
> > > >
> > > >  #include "libcamera/internal/camera_sensor.h"
> > > > @@ -50,6 +52,19 @@ constexpr std::array<PixelFormat, 7> RKISP1_RSZ_MP_FORMATS{
> > > >  	formats::NV12,
> > > >  	/* \todo Add support for 8-bit greyscale to DRM formats */
> > > >  };
> > > > +
> > > > +constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };
> > > > +constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 };
> > >
> > > 1920x1920 ? Maybe it's just the way it actually is
> >
> > At least in the kernel sources.
> >
> > > > +constexpr std::array<PixelFormat, 7> RKISP1_RSZ_SP_FORMATS{
> > > > +	formats::YUYV,
> > > > +	formats::YVYU,
> > > > +	formats::VYUY,
> > > > +	formats::NV16,
> > > > +	formats::NV61,
> > > > +	formats::NV21,
> > > > +	formats::NV12,
> > > > +	/* \todo Add support for BGR888 and RGB565 */
> > > > +};
> > >
> > > Aren't these equal to the main path ones ?
> >
> > Yes and no :-) The enabled ones are, but the todos are not.
> >
> > > >  } /* namespace */
> > > >
> > > >  class PipelineHandlerRkISP1;
> > > > @@ -181,6 +196,14 @@ public:
> > > >  private:
> > > >  	static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
> > > >
> > > > +	CameraConfiguration::Status validatePath(StreamConfiguration *cfg,
> > > > +						 const Span<const PixelFormat> &formats,
> > > > +						 const Size &min, const Size &max,
> > > > +						 V4L2VideoDevice *video);
> > > > +	CameraConfiguration::Status validateMainPath(StreamConfiguration *cfg);
> > > > +	CameraConfiguration::Status validateSelfPath(StreamConfiguration *cfg);
> > > > +	bool fitsAllPaths(const StreamConfiguration &cfg);
> > > > +
> > > >  	/*
> > > >  	 * The RkISP1CameraData instance is guaranteed to be valid as long as the
> > > >  	 * corresponding Camera instance is valid. In order to borrow a
> > > > @@ -492,6 +515,69 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
> > > >  	data_ = data;
> > > >  }
> > > >
> > > > +CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(
> > > > +	StreamConfiguration *cfg, const Span<const PixelFormat> &formats,
> > > > +	const Size &min, const Size &max, V4L2VideoDevice *video)
> > > > +{
> > > > +	const StreamConfiguration reqCfg = *cfg;
> > > > +	Status status = Valid;
> > > > +
> > > > +	if (std::find(formats.begin(), formats.end(), cfg->pixelFormat) ==
> > > > +	    formats.end())
> > > > +		cfg->pixelFormat = formats::NV12;
> > > > +
> > > > +	cfg->size.boundTo(max);
> > > > +	cfg->size.expandTo(min);
> > >
> > > This could be a one liner, but it's a matter of tastes
> >
> > As in prev version I like this much better then to group them in on
> > line.  But as you say it's matter of taste.
> >
> > > > +	cfg->bufferCount = RKISP1_BUFFER_COUNT;
> > > > +
> > > > +	V4L2DeviceFormat format = {};
> > > > +	format.fourcc = video->toV4L2PixelFormat(cfg->pixelFormat);
> > > > +	format.size = cfg->size;
> > > > +
> > > > +	int ret = video->tryFormat(&format);
> > > > +	if (ret)
> > > > +		return Invalid;
> > > > +
> > > > +	cfg->stride = format.planes[0].bpl;
> > > > +	cfg->frameSize = format.planes[0].size;
> > > > +
> > > > +	if (cfg->pixelFormat != reqCfg.pixelFormat || cfg->size != reqCfg.size) {
> > >
> > > Should you check for bufferCount too ?
> >
> > I don't think so, similar to that we don't check stride and frameSize
> > right?
> 
> not sure you know... stride and frameSize are output information,
> bufferCount can actually be set by the application, doesn't it ?
> 
> > > > +		LOG(RkISP1, Debug)
> > > > +			<< "Adjusting format from " << reqCfg.toString()
> > > > +			<< " to " << cfg->toString();
> > > > +		status = Adjusted;
> > > > +	}
> > > > +
> > > > +	return status;
> > > > +}
> > > > +
> > > > +CameraConfiguration::Status RkISP1CameraConfiguration::validateMainPath(StreamConfiguration *cfg)
> > > > +{
> > > > +	return validatePath(cfg, RKISP1_RSZ_MP_FORMATS, RKISP1_RSZ_MP_SRC_MIN,
> > > > +			    RKISP1_RSZ_MP_SRC_MAX, data_->mainPathVideo_);
> > > > +}
> > > > +
> > > > +CameraConfiguration::Status RkISP1CameraConfiguration::validateSelfPath(StreamConfiguration *cfg)
> > > > +{
> > > > +	return validatePath(cfg, RKISP1_RSZ_SP_FORMATS, RKISP1_RSZ_SP_SRC_MIN,
> > > > +			    RKISP1_RSZ_SP_SRC_MAX, data_->selfPathVideo_);
> > > > +}
> > > > +
> > > > +bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)
> > > > +{
> > > > +	StreamConfiguration config;
> > > > +
> > > > +	config = cfg;
> > > > +	if (validateMainPath(&config) != Valid)
> > >
> > > Shouldn't we accept Adjusted too if we want to check if the stream
> > > 'fits' the path ?
> >
> > No.
> >
> > We wish to check for exact match of the requested format. What we wish
> > to learn is if the requested configuration can be satisfied on both
> > paths without being adjusted.
> >
> > > > +		return false;
> > > > +
> > > > +	config = cfg;
> > > > +	if (validateSelfPath(&config) != Valid)
> > > > +		return false;
> > > > +
> > > > +	return true;
> > > > +}
> > > > +
> > > >  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> > > >  {
> > > >  	const CameraSensor *sensor = data_->sensor_;
> > > > @@ -501,22 +587,87 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> > > >  		return Invalid;
> > > >
> > > >  	/* Cap the number of entries to the available streams. */
> > > > -	if (config_.size() > 1) {
> > > > -		config_.resize(1);
> > > > +	if (config_.size() > 2) {
> > > > +		config_.resize(2);
> > > >  		status = Adjusted;
> > > >  	}
> > > >
> > > > -	StreamConfiguration &cfg = config_[0];
> > > > -
> > > > -	/* Adjust the pixel format. */
> > > > -	if (std::find(RKISP1_RSZ_MP_FORMATS.begin(), RKISP1_RSZ_MP_FORMATS.end(),
> > > > -		      cfg.pixelFormat) == RKISP1_RSZ_MP_FORMATS.end()) {
> > > > -		LOG(RkISP1, Debug) << "Adjusting format to NV12";
> > > > -		cfg.pixelFormat = formats::NV12,
> > > > -		status = Adjusted;
> > > > +	/*
> > > > +	 * If there are more then one stream in the configuration figure out the
> > > > +	 * order to evaluate the streams. The first stream has the highest
> > > > +	 * priority but if both main path and self path can satisfy it evaluate
> > > > +	 * second stream first as the first stream is guaranteed to work with
> > > > +	 * whichever path is not used by the second one.
> > > > +	 */
> > > > +	std::vector<unsigned int> order(config_.size());
> > > > +	std::iota(order.begin(), order.end(), 0);
> > > > +	if (config_.size() == 2 && fitsAllPaths(config_[0]))
> > > > +		std::reverse(order.begin(), order.end());
> > >
> > > Let alone the implementation which is nice, is really the order of the
> > > configurations that defines to which output they should be assigned ?
> 
> Asking the same question again, I might have missed where the priority
> assignment requirement comes from.
> 
> > > I'm not familiar with the platform, but in example, I would expect the
> > > larger to go to the main path (as at this time they support the same
> > > formats)
> >
> > No this logic tries to maximize the possibility for both streams to be
> > fully satisfied while still respecting that the first configuration have
> > higher priority if not both can be satisfied.
> >
> > The for-loop bellows assign hardware in a first come first serve fashion
> > and once a path have been assigned the assignment is never re-evaluated.
> > It first tries to see if the configuration can be an exact match on any
> > of the paths and then if it can be adjusted to fit (that is why two
> > passes are needed).
> >
> > Above we check with fitsAllPaths(config_[0]) if the first configuration
> > (with has the highest priority) can fit on any path without being
> > adjusted. If so then we know it's safe to swap the order and of the
> > configurations as to maximize config_[1] to find an exact match on one
> > of the pats. We can do this as we already know config_[0] will have en
> > exact match on either of them.
> >
> > If config_[0] can not find an exact match on both paths we do our best
> > to make it fit first and config_[1] are stick with what ever resources
> > are left.
> >
> > > > +
> > > > +	bool mainPathAvailable = true;
> > > > +	bool selfPathAvailable = true;
> > > > +	for (unsigned int index : order) {
> > > > +		StreamConfiguration &cfg = config_[index];
> > > > +
> > > > +		/* Try to match stream without adjusting configuration. */
> > > > +		if (mainPathAvailable) {
> > > > +			StreamConfiguration tryCfg = cfg;
> > > > +			if (validateMainPath(&tryCfg) == Valid) {
> > > > +				mainPathAvailable = false;
> > > > +				cfg = tryCfg;
> > > > +				cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_));
> > > > +				LOG(RkISP1, Debug) << "Exact match main";
> > > > +				continue;
> > > > +			}
> > > > +		}
> > > > +
> > > > +		if (selfPathAvailable) {
> > > > +			StreamConfiguration tryCfg = cfg;
> > > > +			if (validateSelfPath(&tryCfg) == Valid) {
> > > > +				selfPathAvailable = false;
> > > > +				cfg = tryCfg;
> > >
> > > Do you need to re-assign it if it's valid ?
> >
> > Good point, no it's not needed.
> 
> Wait, maybe bufferCount or stride has changed, as their modification
> doesn't 'Adjust' the stream status.
> 
> > > > +				cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_));
> > > > +				LOG(RkISP1, Debug) << "Exact match self";
> > > > +				continue;
> > > > +			}
> > > > +		}
> > > > +
> > > > +		/* Try to match stream allowing adjusting configuration. */
> > > > +		if (mainPathAvailable) {
> > > > +			StreamConfiguration tryCfg = cfg;
> > > > +			if (validateMainPath(&tryCfg) == Adjusted) {
> > > > +				mainPathAvailable = false;
> > > > +				cfg = tryCfg;
> > > > +				cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_));
> > > > +				status = Adjusted;
> > > > +				LOG(RkISP1, Debug) << "Adjust match main";
> > > > +				continue;
> > > > +			}
> > > > +		}
> > > > +
> > > > +		if (selfPathAvailable) {
> > > > +			StreamConfiguration tryCfg = cfg;
> > > > +			if (validateSelfPath(&tryCfg) == Adjusted) {
> > > > +				selfPathAvailable = false;
> > > > +				cfg = tryCfg;
> > > > +				cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_));
> > > > +				status = Adjusted;
> > > > +				LOG(RkISP1, Debug) << "Adjust match self";
> > >
> > > If I were to read this without reading the code, I would be puzzled.
> > > same for other messages :)
> >
> > I will drop them as they seem to confuse more then help.
> 
> Thanks, I think it could help if expanded. As a reference on IPU3:
> 
>         LOG(IPU3, Debug) << "Assigned " << cfg->toString()
>                          << " to the main output";
> 
> > > > +				continue;
> > > > +			}
> > > > +		}
> > >
> > > I dug out the comment from the review of the first version where I
> > > asked "why" and the answer was that it's done not to adjust a stream
> > > where it is not needed, so you initially only wants "valid" then try
> > > accept "adjusted" if "valid" cannot be obtained.
> > >
> > > So, what are the resons for "adjusting" ? Or either the pixel format
> > > is wrong, and you would need to adjust on both nodes, or if the sizes
> > > are larger the ones supported by the selfpath, in that case you
> > > fallback to the main path and even if in that case they're too large,
> > > adjust the stream. I think you could work this out easily if you sort
> > > streams by size, but as I didn't get where "first as higher priority"
> > > constraints come from, I might be mistaken.
> > >
> > > If size sorted you try with main path first, if it's not taken and you
> > > accept Valid || Adjusted. The second stream goes to the selfpath, and
> > > you accept Valid || Adjusted there too without duplicating the check.
> > >
> > > But if the priority order is justified or even part of the libcamera
> > > API documentation, I think what you have here is fine.
> >
> > If it was only sizes that differed between the two yes, but it's not.
> > The main path can support RAW formats while the self path can support
> > RGB. So sorting by size is unfortunately not possible as we also need to
> > consider the formats.
> 
> But in that case assigning by format is even easier. RAW goes to main
> and RGB goes to self.
> 
> > > Also, I don't know how RAW will work with this pipeline, but will this
> > > double pass scale well in that case or there shouldn't be particular
> > > issues ?
> >
> > RAW will be a special case on this pipeline as it will limit it to one
> > stream. When capturing RAW only the main path can be active.
> >
> > But same question for RGB formats which are only supported by the self
> > path. No this is tested with that in mind as adding BGR888 to the list
> > of supported formats in RKISP1_RSZ_SP_FORMATS is all that is needed to
> > enable it to be captured. But the frames looks a bit wierd in qcam so I
> > will investigate this on top of this series but I do not wish to block
> > this already too large series on also enabeling support for BGR888 and
> > RGB656.
> 
> Agreed on non blocking to wait for RGB.
> 
> > > > +
> > > > +		/* All paths rejected configuraiton. */
> > > > +		LOG(RkISP1, Debug) << "Camera configuration not supported "
> > > > +				   << cfg.toString();
> > > > +		return Invalid;
> > > >  	}
> > > >
> > > >  	/* Select the sensor format. */
> > > > +	Size maxSize;
> > > > +	for (const StreamConfiguration &cfg : config_)
> > > > +		maxSize = std::max(maxSize, cfg.size);
> > > > +
> > > >  	sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,
> > > >  					    MEDIA_BUS_FMT_SGBRG12_1X12,
> > > >  					    MEDIA_BUS_FMT_SGRBG12_1X12,
> > > > @@ -529,47 +680,10 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> > > >  					    MEDIA_BUS_FMT_SGBRG8_1X8,
> > > >  					    MEDIA_BUS_FMT_SGRBG8_1X8,
> > > >  					    MEDIA_BUS_FMT_SRGGB8_1X8 },
> > > > -					  cfg.size);
> > > > +					  maxSize);
> > > >  	if (sensorFormat_.size.isNull())
> > > >  		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.isNull()) {
> > > > -		cfg.size.width = 1280;
> > > > -		cfg.size.height = 1280 * sensorFormat_.size.height
> > > > -				/ sensorFormat_.size.width;
> > > > -	}
> > > > -
> > > > -	cfg.size.boundTo(RKISP1_RSZ_MP_SRC_MAX);
> > > > -	cfg.size.expandTo(RKISP1_RSZ_MP_SRC_MIN);
> > > > -
> > > > -	if (cfg.size != size) {
> > > > -		LOG(RkISP1, Debug)
> > > > -			<< "Adjusting size from " << size.toString()
> > > > -			<< " to " << cfg.size.toString();
> > > > -		status = Adjusted;
> > > > -	}
> > > > -
> > > > -	cfg.bufferCount = RKISP1_BUFFER_COUNT;
> > > > -
> > > > -	V4L2DeviceFormat format = {};
> > > > -	format.fourcc = data_->mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat);
> > > > -	format.size = cfg.size;
> > > > -
> > > > -	int ret = data_->mainPathVideo_->tryFormat(&format);
> > > > -	if (ret)
> > > > -		return Invalid;
> > > > -
> > > > -	cfg.stride = format.planes[0].bpl;
> > > > -	cfg.frameSize = format.planes[0].size;
> > > > -
> > > >  	return status;
> > > >  }
> > > >

Hi Niklas,

Thank you for the patch.

On Fri, Sep 25, 2020 at 03:41:58AM +0200, Niklas Söderlund wrote:
> Extend the format validation to work with both main and self paths. The
> heuristics honors that the first stream in the configuration has the
> highest priority while still examining both streams for a best match.
> 
> It is not possible to capture from the self path as the self stream is
> not yet exposed to applications.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
> * Changes since v2
> - Fix spelling in commit message.
> - Use Span<> instead of turning arrays to vectors.
> - Keep data_ const and cast 'const Streams*' to non-const using
>   const_cast<Stream *>() to match the IPU3 pipeline.
> - Rename fitAnyPath() to fitsAllPaths().
> - Expand documentation for why second stream is evaluated first if the
>   fist stream can use either stream.
> - Drop support for RGB888 and RGB656 for selfpath which was present in
>   v2 as the driver delivers odd data when the frames are observed.
> 
> * Changes since v1
> - Store formats in std::vector instead of std::array to avoid template
>   usage for validate function.
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 210 +++++++++++++++++------
>  1 file changed, 162 insertions(+), 48 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index cd3049485746edd6..bd53183a034efaff 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -9,6 +9,7 @@
>  #include <array>
>  #include <iomanip>
>  #include <memory>
> +#include <numeric>
>  #include <queue>
>  
>  #include <linux/media-bus-format.h>
> @@ -19,6 +20,7 @@
>  #include <libcamera/formats.h>
>  #include <libcamera/ipa/rkisp1.h>
>  #include <libcamera/request.h>
> +#include <libcamera/span.h>
>  #include <libcamera/stream.h>
>  
>  #include "libcamera/internal/camera_sensor.h"
> @@ -50,6 +52,19 @@ constexpr std::array<PixelFormat, 7> RKISP1_RSZ_MP_FORMATS{
>  	formats::NV12,
>  	/* \todo Add support for 8-bit greyscale to DRM formats */
>  };
> +
> +constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };
> +constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 };
> +constexpr std::array<PixelFormat, 7> RKISP1_RSZ_SP_FORMATS{
> +	formats::YUYV,
> +	formats::YVYU,
> +	formats::VYUY,
> +	formats::NV16,
> +	formats::NV61,
> +	formats::NV21,
> +	formats::NV12,
> +	/* \todo Add support for BGR888 and RGB565 */
> +};
>  } /* namespace */
>  
>  class PipelineHandlerRkISP1;
> @@ -181,6 +196,14 @@ public:
>  private:
>  	static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
>  
> +	CameraConfiguration::Status validatePath(StreamConfiguration *cfg,
> +						 const Span<const PixelFormat> &formats,
> +						 const Size &min, const Size &max,
> +						 V4L2VideoDevice *video);
> +	CameraConfiguration::Status validateMainPath(StreamConfiguration *cfg);
> +	CameraConfiguration::Status validateSelfPath(StreamConfiguration *cfg);
> +	bool fitsAllPaths(const StreamConfiguration &cfg);
> +
>  	/*
>  	 * The RkISP1CameraData instance is guaranteed to be valid as long as the
>  	 * corresponding Camera instance is valid. In order to borrow a
> @@ -492,6 +515,69 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
>  	data_ = data;
>  }
>  
> +CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(
> +	StreamConfiguration *cfg, const Span<const PixelFormat> &formats,
> +	const Size &min, const Size &max, V4L2VideoDevice *video)
> +{
> +	const StreamConfiguration reqCfg = *cfg;
> +	Status status = Valid;
> +
> +	if (std::find(formats.begin(), formats.end(), cfg->pixelFormat) ==
> +	    formats.end())
> +		cfg->pixelFormat = formats::NV12;
> +
> +	cfg->size.boundTo(max);
> +	cfg->size.expandTo(min);
> +	cfg->bufferCount = RKISP1_BUFFER_COUNT;
> +
> +	V4L2DeviceFormat format = {};
> +	format.fourcc = video->toV4L2PixelFormat(cfg->pixelFormat);
> +	format.size = cfg->size;
> +
> +	int ret = video->tryFormat(&format);
> +	if (ret)
> +		return Invalid;
> +
> +	cfg->stride = format.planes[0].bpl;
> +	cfg->frameSize = format.planes[0].size;
> +
> +	if (cfg->pixelFormat != reqCfg.pixelFormat || cfg->size != reqCfg.size) {
> +		LOG(RkISP1, Debug)
> +			<< "Adjusting format from " << reqCfg.toString()
> +			<< " to " << cfg->toString();
> +		status = Adjusted;
> +	}
> +
> +	return status;
> +}
> +
> +CameraConfiguration::Status RkISP1CameraConfiguration::validateMainPath(StreamConfiguration *cfg)
> +{
> +	return validatePath(cfg, RKISP1_RSZ_MP_FORMATS, RKISP1_RSZ_MP_SRC_MIN,
> +			    RKISP1_RSZ_MP_SRC_MAX, data_->mainPathVideo_);
> +}
> +
> +CameraConfiguration::Status RkISP1CameraConfiguration::validateSelfPath(StreamConfiguration *cfg)
> +{
> +	return validatePath(cfg, RKISP1_RSZ_SP_FORMATS, RKISP1_RSZ_SP_SRC_MIN,
> +			    RKISP1_RSZ_SP_SRC_MAX, data_->selfPathVideo_);
> +}
> +
> +bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)
> +{
> +	StreamConfiguration config;
> +
> +	config = cfg;
> +	if (validateMainPath(&config) != Valid)
> +		return false;
> +
> +	config = cfg;
> +	if (validateSelfPath(&config) != Valid)
> +		return false;
> +
> +	return true;
> +}
> +
>  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  {
>  	const CameraSensor *sensor = data_->sensor_;
> @@ -501,22 +587,87 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  		return Invalid;
>  
>  	/* Cap the number of entries to the available streams. */
> -	if (config_.size() > 1) {
> -		config_.resize(1);
> +	if (config_.size() > 2) {
> +		config_.resize(2);
>  		status = Adjusted;
>  	}
>  
> -	StreamConfiguration &cfg = config_[0];
> -
> -	/* Adjust the pixel format. */
> -	if (std::find(RKISP1_RSZ_MP_FORMATS.begin(), RKISP1_RSZ_MP_FORMATS.end(),
> -		      cfg.pixelFormat) == RKISP1_RSZ_MP_FORMATS.end()) {
> -		LOG(RkISP1, Debug) << "Adjusting format to NV12";
> -		cfg.pixelFormat = formats::NV12,
> -		status = Adjusted;
> +	/*
> +	 * If there are more then one stream in the configuration figure out the

s/then/than/

> +	 * order to evaluate the streams. The first stream has the highest
> +	 * priority but if both main path and self path can satisfy it evaluate
> +	 * second stream first as the first stream is guaranteed to work with

s/second/the second/

> +	 * whichever path is not used by the second one.
> +	 */
> +	std::vector<unsigned int> order(config_.size());
> +	std::iota(order.begin(), order.end(), 0);
> +	if (config_.size() == 2 && fitsAllPaths(config_[0]))
> +		std::reverse(order.begin(), order.end());
> +
> +	bool mainPathAvailable = true;
> +	bool selfPathAvailable = true;
> +	for (unsigned int index : order) {
> +		StreamConfiguration &cfg = config_[index];
> +
> +		/* Try to match stream without adjusting configuration. */
> +		if (mainPathAvailable) {
> +			StreamConfiguration tryCfg = cfg;
> +			if (validateMainPath(&tryCfg) == Valid) {
> +				mainPathAvailable = false;
> +				cfg = tryCfg;
> +				cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_));
> +				LOG(RkISP1, Debug) << "Exact match main";

I'd either drop the message, or make it more explicit. Something like
this maybe.

				LOG(RkISP1, Debug)
					<< "Stream " << index
					<< " configuration matches main path exactly";

Same below.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +				continue;
> +			}
> +		}
> +
> +		if (selfPathAvailable) {
> +			StreamConfiguration tryCfg = cfg;
> +			if (validateSelfPath(&tryCfg) == Valid) {
> +				selfPathAvailable = false;
> +				cfg = tryCfg;
> +				cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_));
> +				LOG(RkISP1, Debug) << "Exact match self";
> +				continue;
> +			}
> +		}
> +
> +		/* Try to match stream allowing adjusting configuration. */
> +		if (mainPathAvailable) {
> +			StreamConfiguration tryCfg = cfg;
> +			if (validateMainPath(&tryCfg) == Adjusted) {
> +				mainPathAvailable = false;
> +				cfg = tryCfg;
> +				cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_));
> +				status = Adjusted;
> +				LOG(RkISP1, Debug) << "Adjust match main";
> +				continue;
> +			}
> +		}
> +
> +		if (selfPathAvailable) {
> +			StreamConfiguration tryCfg = cfg;
> +			if (validateSelfPath(&tryCfg) == Adjusted) {
> +				selfPathAvailable = false;
> +				cfg = tryCfg;
> +				cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_));
> +				status = Adjusted;
> +				LOG(RkISP1, Debug) << "Adjust match self";
> +				continue;
> +			}
> +		}
> +
> +		/* All paths rejected configuraiton. */
> +		LOG(RkISP1, Debug) << "Camera configuration not supported "
> +				   << cfg.toString();
> +		return Invalid;
>  	}
>  
>  	/* Select the sensor format. */
> +	Size maxSize;
> +	for (const StreamConfiguration &cfg : config_)
> +		maxSize = std::max(maxSize, cfg.size);
> +
>  	sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,
>  					    MEDIA_BUS_FMT_SGBRG12_1X12,
>  					    MEDIA_BUS_FMT_SGRBG12_1X12,
> @@ -529,47 +680,10 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  					    MEDIA_BUS_FMT_SGBRG8_1X8,
>  					    MEDIA_BUS_FMT_SGRBG8_1X8,
>  					    MEDIA_BUS_FMT_SRGGB8_1X8 },
> -					  cfg.size);
> +					  maxSize);
>  	if (sensorFormat_.size.isNull())
>  		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.isNull()) {
> -		cfg.size.width = 1280;
> -		cfg.size.height = 1280 * sensorFormat_.size.height
> -				/ sensorFormat_.size.width;
> -	}
> -
> -	cfg.size.boundTo(RKISP1_RSZ_MP_SRC_MAX);
> -	cfg.size.expandTo(RKISP1_RSZ_MP_SRC_MIN);
> -
> -	if (cfg.size != size) {
> -		LOG(RkISP1, Debug)
> -			<< "Adjusting size from " << size.toString()
> -			<< " to " << cfg.size.toString();
> -		status = Adjusted;
> -	}
> -
> -	cfg.bufferCount = RKISP1_BUFFER_COUNT;
> -
> -	V4L2DeviceFormat format = {};
> -	format.fourcc = data_->mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat);
> -	format.size = cfg.size;
> -
> -	int ret = data_->mainPathVideo_->tryFormat(&format);
> -	if (ret)
> -		return Invalid;
> -
> -	cfg.stride = format.planes[0].bpl;
> -	cfg.frameSize = format.planes[0].size;
> -
>  	return status;
>  }
>