[libcamera-devel,v2,12/13] libcamera: pipeline: rkisp1: Add format validation for self path
diff mbox series

Message ID 20200914142149.63857-13-niklas.soderlund@ragnatech.se
State Accepted
Headers show
Series
  • libcamera: pipeline: rkisp1: Extend to support two streams
Related show

Commit Message

Niklas Söderlund Sept. 14, 2020, 2:21 p.m. UTC 
Extend the format validation to work with both man and self path. The
heuristics honors that the first stream in the configuration have the
highest priority while still examining both streams for a best match.

This change extends the formats supported by the Cameras backed by this
pipeline to also include RGB888 and RGB656, that is only available on
the self path.

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 v1
- Store formats in std::vector instead of std::array to avoid template
  usage for validate function.
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 222 +++++++++++++++++------
 1 file changed, 171 insertions(+), 51 deletions(-)

Comments

Laurent Pinchart Sept. 15, 2020, 1:32 a.m. UTC | #1 
Hi Niklas,

Thank you for the patch.

On Mon, Sep 14, 2020 at 04:21:48PM +0200, Niklas Söderlund wrote:
> Extend the format validation to work with both man and self path. The

s/man/main/
s/path/paths/

> heuristics honors that the first stream in the configuration have the

s/have/has/

> highest priority while still examining both streams for a best match.
> 
> This change extends the formats supported by the Cameras backed by this

s/Cameras/cameras/

> pipeline to also include RGB888 and RGB656, that is only available on
> the self path.
> 
> 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 v1
> - Store formats in std::vector instead of std::array to avoid template
>   usage for validate function.

As commented (too late) on v1, you don't have to do this, you can keep
an array, and pass a Span<PixelFormat> to validate().

> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 222 +++++++++++++++++------
>  1 file changed, 171 insertions(+), 51 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index bc961f8e78f2c979..851ff68f138b98dd 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>
> @@ -40,7 +41,7 @@ LOG_DEFINE_CATEGORY(RkISP1)
>  namespace {
>  	constexpr Size RKISP1_RSZ_MP_SRC_MIN { 32, 16 };
>  	constexpr Size RKISP1_RSZ_MP_SRC_MAX { 4416, 3312 };
> -	constexpr std::array<PixelFormat, 7> RKISP1_RSZ_MP_FORMATS{
> +	const std::vector<PixelFormat> RKISP1_RSZ_MP_FORMATS{
>  		formats::YUYV,
>  		formats::YVYU,
>  		formats::VYUY,
> @@ -50,7 +51,21 @@ namespace {
>  		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 };
> +	const std::vector<PixelFormat> RKISP1_RSZ_SP_FORMATS{
> +		formats::YUYV,
> +		formats::YVYU,
> +		formats::VYUY,
> +		formats::NV16,
> +		formats::NV61,
> +		formats::NV21,
> +		formats::NV12,
> +		formats::BGR888,
> +		formats::RGB565,
> +	};
> +};
>  
>  class PipelineHandlerRkISP1;
>  class RkISP1ActionQueueBuffers;
> @@ -181,13 +196,22 @@ public:
>  private:
>  	static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
>  
> +	bool fitAnyPath(const StreamConfiguration &cfg);
> +
> +	CameraConfiguration::Status validatePath(StreamConfiguration *cfg,
> +						 const std::vector<PixelFormat> &formats,
> +						 const Size &min, const Size &max,
> +						 V4L2VideoDevice *video);
> +	CameraConfiguration::Status validateMainPath(StreamConfiguration *cfg);
> +	CameraConfiguration::Status validateSelfPath(StreamConfiguration *cfg);
> +
>  	/*
>  	 * 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_;
> +	RkISP1CameraData *data_;

This isn't nice :-( I'd like to find a wait to keep it const. Can be
done in a separate patch.

>  
>  	V4L2SubdeviceFormat sensorFormat_;
>  };
> @@ -497,6 +521,75 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
>  	data_ = data;
>  }
>  
> +bool RkISP1CameraConfiguration::fitAnyPath(const StreamConfiguration &cfg)
> +{
> +	if (std::find(RKISP1_RSZ_MP_FORMATS.begin(),
> +		      RKISP1_RSZ_MP_FORMATS.end(), cfg.pixelFormat) ==
> +	    RKISP1_RSZ_MP_FORMATS.end())
> +		return false;
> +
> +	if (cfg.size < RKISP1_RSZ_MP_SRC_MIN || cfg.size > RKISP1_RSZ_MP_SRC_MAX)
> +		return false;
> +
> +	if (std::find(RKISP1_RSZ_SP_FORMATS.begin(),
> +		      RKISP1_RSZ_SP_FORMATS.end(), cfg.pixelFormat) ==
> +	    RKISP1_RSZ_SP_FORMATS.end())
> +		return false;
> +
> +	if (cfg.size < RKISP1_RSZ_SP_SRC_MIN || cfg.size > RKISP1_RSZ_SP_SRC_MAX)
> +		return false;
> +
> +	return true;

The implementation looks like a fitsAllPaths(), not any path.

> +}
> +
> +CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(
> +	StreamConfiguration *cfg, const std::vector<PixelFormat> &formats,
> +	const Size &min, const Size &max, V4L2VideoDevice *video)

formats, min and max are also candidates to be stored in the RkISP1Path
class.

> +{
> +	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_);
> +}

And those two functions could then disappear. Are you really sure you
want to address all that on top ?

> +
>  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  {
>  	const CameraSensor *sensor = data_->sensor_;
> @@ -506,22 +599,86 @@ 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 them streams. The first stream have the highest

"them streams" ?
s/have/has/

> +	 * priority but if both main path and self path can satisfy it evaluate
> +	 * second stream first.

Why ?

> +	 */
> +	std::vector<unsigned int> order(config_.size());
> +	std::iota(order.begin(), order.end(), 0);
> +	if (config_.size() == 2 && fitAnyPath(config_[0]))
> +		std::reverse(order.begin(), order.end());

A bit complicated for a vector of two elements :-)

> +
> +	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(&data_->mainPathStream_);
> +				LOG(RkISP1, Debug) << "Exact match main";

The message is a bit cryptic for someone reading the log without knowing
the code.

> +				continue;
> +			}
> +		}
> +
> +		if (selfPathAvailable) {
> +			StreamConfiguration tryCfg = cfg;
> +			if (validateSelfPath(&tryCfg) == Valid) {
> +				selfPathAvailable = false;
> +				cfg = tryCfg;
> +				cfg.setStream(&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(&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(&data_->selfPathStream_);
> +				status = Adjusted;
> +				LOG(RkISP1, Debug) << "Adjust match self";
> +				continue;
> +			}
> +		}

There's lots of code duplication, adding a RkISP1Path class would help.

> +
> +		/* 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,
> @@ -534,47 +691,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;
>  }
>
Niklas Söderlund Sept. 22, 2020, 2:48 p.m. UTC | #2 
Hi Laurent,

Thanks for your feedback.

On 2020-09-15 04:32:17 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Mon, Sep 14, 2020 at 04:21:48PM +0200, Niklas Söderlund wrote:
> > Extend the format validation to work with both man and self path. The
> 
> s/man/main/
> s/path/paths/
> 
> > heuristics honors that the first stream in the configuration have the
> 
> s/have/has/
> 
> > highest priority while still examining both streams for a best match.
> > 
> > This change extends the formats supported by the Cameras backed by this
> 
> s/Cameras/cameras/
> 
> > pipeline to also include RGB888 and RGB656, that is only available on
> > the self path.
> > 
> > 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 v1
> > - Store formats in std::vector instead of std::array to avoid template
> >   usage for validate function.
> 
> As commented (too late) on v1, you don't have to do this, you can keep
> an array, and pass a Span<PixelFormat> to validate().
> 
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 222 +++++++++++++++++------
> >  1 file changed, 171 insertions(+), 51 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index bc961f8e78f2c979..851ff68f138b98dd 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>
> > @@ -40,7 +41,7 @@ LOG_DEFINE_CATEGORY(RkISP1)
> >  namespace {
> >  	constexpr Size RKISP1_RSZ_MP_SRC_MIN { 32, 16 };
> >  	constexpr Size RKISP1_RSZ_MP_SRC_MAX { 4416, 3312 };
> > -	constexpr std::array<PixelFormat, 7> RKISP1_RSZ_MP_FORMATS{
> > +	const std::vector<PixelFormat> RKISP1_RSZ_MP_FORMATS{
> >  		formats::YUYV,
> >  		formats::YVYU,
> >  		formats::VYUY,
> > @@ -50,7 +51,21 @@ namespace {
> >  		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 };
> > +	const std::vector<PixelFormat> RKISP1_RSZ_SP_FORMATS{
> > +		formats::YUYV,
> > +		formats::YVYU,
> > +		formats::VYUY,
> > +		formats::NV16,
> > +		formats::NV61,
> > +		formats::NV21,
> > +		formats::NV12,
> > +		formats::BGR888,
> > +		formats::RGB565,
> > +	};
> > +};
> >  
> >  class PipelineHandlerRkISP1;
> >  class RkISP1ActionQueueBuffers;
> > @@ -181,13 +196,22 @@ public:
> >  private:
> >  	static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
> >  
> > +	bool fitAnyPath(const StreamConfiguration &cfg);
> > +
> > +	CameraConfiguration::Status validatePath(StreamConfiguration *cfg,
> > +						 const std::vector<PixelFormat> &formats,
> > +						 const Size &min, const Size &max,
> > +						 V4L2VideoDevice *video);
> > +	CameraConfiguration::Status validateMainPath(StreamConfiguration *cfg);
> > +	CameraConfiguration::Status validateSelfPath(StreamConfiguration *cfg);
> > +
> >  	/*
> >  	 * 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_;
> > +	RkISP1CameraData *data_;
> 
> This isn't nice :-( I'd like to find a wait to keep it const. Can be
> done in a separate patch.

I really tried to keep this const but all I can think of is the fetch 
the Stream* from camera_ instead of data_ in the rather odd hack,

       Stream *mainStream = nullptr;
       Stream *selfStream = nullptr;
       for (Stream *stream : camera_->streams()) {
               if (stream == &data_->mainPathStream_)
                       mainStream = stream;
               else if (stream == &data_->selfPathStream_)
                       selfStream = stream;
       }
       ASSERT(mainStream && selfStream);

But this seems like casting a const variable to a non-const with extra 
steps. Looking at the IPU3 pipeline handler this is exactly what is 
done, from IPU3CameraConfiguration::validate():

    cfg->setStream(const_cast<Stream *>(&data_->outStream_));

I think this highlights an issue with our overall design and something 
that should be corrected. As this transient more pipeline handlers I 
will record this as a todo and use the IPU3 pattern here to make it easy 
to find a solution which solves the root problem.

> 
> >  
> >  	V4L2SubdeviceFormat sensorFormat_;
> >  };
> > @@ -497,6 +521,75 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
> >  	data_ = data;
> >  }
> >  
> > +bool RkISP1CameraConfiguration::fitAnyPath(const StreamConfiguration &cfg)
> > +{
> > +	if (std::find(RKISP1_RSZ_MP_FORMATS.begin(),
> > +		      RKISP1_RSZ_MP_FORMATS.end(), cfg.pixelFormat) ==
> > +	    RKISP1_RSZ_MP_FORMATS.end())
> > +		return false;
> > +
> > +	if (cfg.size < RKISP1_RSZ_MP_SRC_MIN || cfg.size > RKISP1_RSZ_MP_SRC_MAX)
> > +		return false;
> > +
> > +	if (std::find(RKISP1_RSZ_SP_FORMATS.begin(),
> > +		      RKISP1_RSZ_SP_FORMATS.end(), cfg.pixelFormat) ==
> > +	    RKISP1_RSZ_SP_FORMATS.end())
> > +		return false;
> > +
> > +	if (cfg.size < RKISP1_RSZ_SP_SRC_MIN || cfg.size > RKISP1_RSZ_SP_SRC_MAX)
> > +		return false;
> > +
> > +	return true;
> 
> The implementation looks like a fitsAllPaths(), not any path.
> 
> > +}
> > +
> > +CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(
> > +	StreamConfiguration *cfg, const std::vector<PixelFormat> &formats,
> > +	const Size &min, const Size &max, V4L2VideoDevice *video)
> 
> formats, min and max are also candidates to be stored in the RkISP1Path
> class.
> 
> > +{
> > +	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_);
> > +}
> 
> And those two functions could then disappear. Are you really sure you
> want to address all that on top ?

That is my current plan. But to extinguish any doubt of that work I will 
do it on-top now and submit it as part of v3.

> 
> > +
> >  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> >  {
> >  	const CameraSensor *sensor = data_->sensor_;
> > @@ -506,22 +599,86 @@ 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 them streams. The first stream have the highest
> 
> "them streams" ?
> s/have/has/
> 
> > +	 * priority but if both main path and self path can satisfy it evaluate
> > +	 * second stream first.
> 
> Why ?
> 
> > +	 */
> > +	std::vector<unsigned int> order(config_.size());
> > +	std::iota(order.begin(), order.end(), 0);
> > +	if (config_.size() == 2 && fitAnyPath(config_[0]))
> > +		std::reverse(order.begin(), order.end());
> 
> A bit complicated for a vector of two elements :-)
> 
> > +
> > +	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(&data_->mainPathStream_);
> > +				LOG(RkISP1, Debug) << "Exact match main";
> 
> The message is a bit cryptic for someone reading the log without knowing
> the code.

Sure, but is not the Debug level intended for people debugging the code?  
I would be fine dropping the LOG() statements all together. I found them 
useful when testing this series and chased to leave them in.

> 
> > +				continue;
> > +			}
> > +		}
> > +
> > +		if (selfPathAvailable) {
> > +			StreamConfiguration tryCfg = cfg;
> > +			if (validateSelfPath(&tryCfg) == Valid) {
> > +				selfPathAvailable = false;
> > +				cfg = tryCfg;
> > +				cfg.setStream(&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(&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(&data_->selfPathStream_);
> > +				status = Adjusted;
> > +				LOG(RkISP1, Debug) << "Adjust match self";
> > +				continue;
> > +			}
> > +		}
> 
> There's lots of code duplication, adding a RkISP1Path class would help.
> 
> > +
> > +		/* 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,
> > @@ -534,47 +691,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;
> >  }
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index bc961f8e78f2c979..851ff68f138b98dd 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>
@@ -40,7 +41,7 @@  LOG_DEFINE_CATEGORY(RkISP1)
 namespace {
 	constexpr Size RKISP1_RSZ_MP_SRC_MIN { 32, 16 };
 	constexpr Size RKISP1_RSZ_MP_SRC_MAX { 4416, 3312 };
-	constexpr std::array<PixelFormat, 7> RKISP1_RSZ_MP_FORMATS{
+	const std::vector<PixelFormat> RKISP1_RSZ_MP_FORMATS{
 		formats::YUYV,
 		formats::YVYU,
 		formats::VYUY,
@@ -50,7 +51,21 @@  namespace {
 		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 };
+	const std::vector<PixelFormat> RKISP1_RSZ_SP_FORMATS{
+		formats::YUYV,
+		formats::YVYU,
+		formats::VYUY,
+		formats::NV16,
+		formats::NV61,
+		formats::NV21,
+		formats::NV12,
+		formats::BGR888,
+		formats::RGB565,
+	};
+};
 
 class PipelineHandlerRkISP1;
 class RkISP1ActionQueueBuffers;
@@ -181,13 +196,22 @@  public:
 private:
 	static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
 
+	bool fitAnyPath(const StreamConfiguration &cfg);
+
+	CameraConfiguration::Status validatePath(StreamConfiguration *cfg,
+						 const std::vector<PixelFormat> &formats,
+						 const Size &min, const Size &max,
+						 V4L2VideoDevice *video);
+	CameraConfiguration::Status validateMainPath(StreamConfiguration *cfg);
+	CameraConfiguration::Status validateSelfPath(StreamConfiguration *cfg);
+
 	/*
 	 * 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_;
+	RkISP1CameraData *data_;
 
 	V4L2SubdeviceFormat sensorFormat_;
 };
@@ -497,6 +521,75 @@  RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
 	data_ = data;
 }
 
+bool RkISP1CameraConfiguration::fitAnyPath(const StreamConfiguration &cfg)
+{
+	if (std::find(RKISP1_RSZ_MP_FORMATS.begin(),
+		      RKISP1_RSZ_MP_FORMATS.end(), cfg.pixelFormat) ==
+	    RKISP1_RSZ_MP_FORMATS.end())
+		return false;
+
+	if (cfg.size < RKISP1_RSZ_MP_SRC_MIN || cfg.size > RKISP1_RSZ_MP_SRC_MAX)
+		return false;
+
+	if (std::find(RKISP1_RSZ_SP_FORMATS.begin(),
+		      RKISP1_RSZ_SP_FORMATS.end(), cfg.pixelFormat) ==
+	    RKISP1_RSZ_SP_FORMATS.end())
+		return false;
+
+	if (cfg.size < RKISP1_RSZ_SP_SRC_MIN || cfg.size > RKISP1_RSZ_SP_SRC_MAX)
+		return false;
+
+	return true;
+}
+
+CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(
+	StreamConfiguration *cfg, const std::vector<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_);
+}
+
 CameraConfiguration::Status RkISP1CameraConfiguration::validate()
 {
 	const CameraSensor *sensor = data_->sensor_;
@@ -506,22 +599,86 @@  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 them streams. The first stream have the highest
+	 * priority but if both main path and self path can satisfy it evaluate
+	 * second stream first.
+	 */
+	std::vector<unsigned int> order(config_.size());
+	std::iota(order.begin(), order.end(), 0);
+	if (config_.size() == 2 && fitAnyPath(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(&data_->mainPathStream_);
+				LOG(RkISP1, Debug) << "Exact match main";
+				continue;
+			}
+		}
+
+		if (selfPathAvailable) {
+			StreamConfiguration tryCfg = cfg;
+			if (validateSelfPath(&tryCfg) == Valid) {
+				selfPathAvailable = false;
+				cfg = tryCfg;
+				cfg.setStream(&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(&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(&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,
@@ -534,47 +691,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;
 }