[libcamera-devel] [PATCH v3 13/22] libcamera: pipeline: rkisp1: Add format validation for self path
Niklas Söderlund
niklas.soderlund at ragnatech.se
Fri Sep 25 18:48:25 CEST 2020
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 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?
>
> > + 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 at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list