[PATCH] pipeline: rkisp1: Fix validation when sensor max is larger than ISP input
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Aug 2 21:03:56 CEST 2024
Hi Umang and Paul,
Thank you for the patch.
On Wed, Jul 24, 2024 at 03:58:12PM +0530, Umang Jain wrote:
> From: Paul Elder <paul.elder at ideasonboard.com>
>
> If the maximum sensor output size is larger than the maximum ISP input
> size, the maximum sensor size could be selected and the pipeline would
> fail with an EPIPE. Fix this by validating a suitable sensor output size
> which is less than or equal to, the ISP input.
s/to,/to/
(or "less than, or equal to,")
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
> Split out from https://patchwork.libcamera.org/project/libcamera/list/?series=4143
>
> Changes in v2:
> - trivial var rename
> - Properly obtain a resolution from sensor supported for ISP max input
> - Refactor slightly to fit better
> ---
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 ++++++-
> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 51 +++++++++++++------
> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 5 +-
> 3 files changed, 55 insertions(+), 18 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 4cbf105d..5f94f422 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -1202,11 +1202,24 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
> if (param_->open() < 0)
> return false;
>
> + /*
> + * Retrieve the ISP maximum input size for config validation in the
> + * path classes.
> + */
> + Size ispMaxInputSize;
> + V4L2Subdevice::Formats ispFormats = isp_->formats(0);
const if you don't need to modify this.
> + for (const auto &[mbus, sizes] : ispFormats) {
> + for (const auto &size : sizes) {
> + if (ispMaxInputSize < size.max)
> + ispMaxInputSize = size.max;
> + }
> + }
I've left a review comment on this in v1, please address it or reply to
it if you think it's wrong.
> +
> /* Locate and open the ISP main and self paths. */
> - if (!mainPath_.init(media_))
> + if (!mainPath_.init(media_, ispMaxInputSize))
> return false;
>
> - if (hasSelfPath_ && !selfPath_.init(media_))
> + if (hasSelfPath_ && !selfPath_.init(media_, ispMaxInputSize))
> return false;
>
> mainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index c49017d1..6b40cdd2 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -62,7 +62,7 @@ RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats,
> {
> }
>
> -bool RkISP1Path::init(MediaDevice *media)
> +bool RkISP1Path::init(MediaDevice *media, const Size &ispMaxInputSize)
> {
> std::string resizer = std::string("rkisp1_resizer_") + name_ + "path";
> std::string video = std::string("rkisp1_") + name_ + "path";
> @@ -75,6 +75,7 @@ bool RkISP1Path::init(MediaDevice *media)
> if (video_->open() < 0)
> return false;
>
> + ispMaxInputSize_ = ispMaxInputSize;
> populateFormats();
>
> link_ = media->link("rkisp1_isp", 2, resizer, 0);
> @@ -126,12 +127,33 @@ void RkISP1Path::populateFormats()
> }
> }
>
> +Size RkISP1Path::maxSupportedSensorResolution(const CameraSensor *sensor)
> +{
> + Size sensorResolution;
> +
> + /* Get highest sensor resolution which is just less than or equal to ISP input */
> + for (const auto &format : streamFormats_) {
> + auto sizes = sensor->sizes(formatToMediaBus.at(format));
Are you sure ? streamFormats_ contains the formats supported on the ISP
output. Translating that to a media bus code and then considering it
matches the sensor format doesn't make much sense to me.
> + for (auto &sz : sizes) {
> + if (sz <= ispMaxInputSize_ && sz > sensorResolution)
I don't think this is right, see how the comparison operator is defined
for the Size class.
> + sensorResolution = sz;
> + }
> + }
> +
> + return sensorResolution;
> +}
> +
> StreamConfiguration
> RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,
> StreamRole role)
> {
> const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
> - const Size &resolution = sensor->resolution();
> + Size resolution = maxSupportedSensorResolution(sensor);
This is a sensor invariant, isn't it ? Why do you recompute it every
time a configuration is generate ?
> + if (resolution.isNull()) {
> + LOG(RkISP1, Error) << "No suitable format/resolution found"
> + << "for ISP input";
> + return {};
> + }
>
> /* Min and max resolutions to populate the available stream formats. */
> Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> @@ -220,7 +242,12 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
> StreamConfiguration *cfg)
> {
> const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
> - const Size &resolution = sensor->resolution();
> + Size resolution = maxSupportedSensorResolution(sensor);
And validated too.
And furthermore, the maximum supported sensor resolution should be the
same for both paths, shouldn't it ?
Overall this patch feels a bit too much of a hack for me to be
comfortable with it.
> + if (resolution.isNull()) {
> + LOG(RkISP1, Error) << "No suitable format/resolution found"
> + << "for ISP input";
> + return {};
> + }
>
> const StreamConfiguration reqCfg = *cfg;
> CameraConfiguration::Status status = CameraConfiguration::Valid;
> @@ -275,8 +302,8 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
> if (!found)
> cfg->pixelFormat = isRaw ? rawFormat : formats::NV12;
>
> - Size minResolution;
> - Size maxResolution;
> + Size maxResolution = maxResolution_.boundedTo(resolution);
> + Size minResolution = minResolution_.expandedToAspectRatio(resolution);
>
> if (isRaw) {
> /*
> @@ -287,16 +314,10 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
> V4L2SubdeviceFormat sensorFormat =
> sensor->getFormat({ mbusCode }, cfg->size);
>
> - minResolution = sensorFormat.size;
> - maxResolution = sensorFormat.size;
> - } else {
> - /*
> - * Adjust the size based on the sensor resolution and absolute
> - * limits of the ISP.
> - */
> - minResolution = minResolution_.expandedToAspectRatio(resolution);
> - maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> - .boundedTo(resolution);
> + if (!sensorFormat.size.isNull()) {
> + minResolution = sensorFormat.size.boundedTo(ispMaxInputSize_);
> + maxResolution = sensorFormat.size.boundedTo(ispMaxInputSize_);
> + }
> }
>
> cfg->size.boundTo(maxResolution);
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> index 08edefec..a9bcfe36 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> @@ -35,7 +35,7 @@ public:
> RkISP1Path(const char *name, const Span<const PixelFormat> &formats,
> const Size &minResolution, const Size &maxResolution);
>
> - bool init(MediaDevice *media);
> + bool init(MediaDevice *media, const Size &ispMaxInputSize);
>
> int setEnabled(bool enable) { return link_->setEnabled(enable); }
> bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }
> @@ -63,6 +63,7 @@ public:
>
> private:
> void populateFormats();
> + Size maxSupportedSensorResolution(const CameraSensor *sensor);
>
> static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
>
> @@ -77,6 +78,8 @@ private:
> std::unique_ptr<V4L2Subdevice> resizer_;
> std::unique_ptr<V4L2VideoDevice> video_;
> MediaLink *link_;
> +
> + Size ispMaxInputSize_;
> };
>
> class RkISP1MainPath : public RkISP1Path
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list