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

[Laurent Pinchart]

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 at 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;
> > > >  }
> > > >

-- 
Regards,

Laurent Pinchart