[libcamera-devel,2/3] pipeline: rkisp1: Fix validation when sensor max is larger than ISP input

19411 diff mbox series

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 clamping the sensor size to the ISP
input size at validation time.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 17 ++++++++++--
 src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 26 +++++++++----------
 src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  4 ++-
 3 files changed, 30 insertions(+), 17 deletions(-)

Quoting Paul Elder via libcamera-devel (2024-01-16 09:17:53)
> 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 clamping the sensor size to the ISP
> input size at validation time.

I think we should do more here (in a separate patch) to filter out
sensor modes that are larger than the ISP max as well.

At the moment it seems too easy for the rkisp1 pipeline handler to
select an input configuration that it can't actually configure.

But that's aside from this patch itself.

> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 17 ++++++++++--
>  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 26 +++++++++----------
>  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  4 ++-
>  3 files changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 586b46d64..fb67ba8f4 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 ISP maximum input size for config validation in the path
> +        * classes
> +        */
> +       Size ispMaxInSize = { 0, 0 };
> +       V4L2Subdevice::Formats ispFormats = isp_->formats(0);
> +       for (const auto &[mbus, sizes] : ispFormats) {
> +               for (const auto &size : sizes) {
> +                       if (ispMaxInSize < size.max)
> +                               ispMaxInSize = size.max;
> +               }
> +       }
> +
>         /* Locate and open the ISP main and self paths. */
> -       if (!mainPath_.init(media_))
> +       if (!mainPath_.init(media_, ispMaxInSize))
>                 return false;
>  
> -       if (hasSelfPath_ && !selfPath_.init(media_))
> +       if (hasSelfPath_ && !selfPath_.init(media_, ispMaxInSize))
>                 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 b62b645ca..eaab7e857 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 &ispMaxInSize)

It feels a bit odd to me passing ispMaxInSize as the only parameter to
initialize. But I guess it save init from re-determining it for each
path, and maybe it's the only variable for now - so I think we're ok
here.

I wonder if we should iterate and 'try' configurations to only expose
those which can be supported. But we can tackle that on top I think.

So for now I am ok with this:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>  {
>         std::string resizer = std::string("rkisp1_resizer_") + name_ + "path";
>         std::string video = std::string("rkisp1_") + name_ + "path";
> @@ -76,6 +76,7 @@ bool RkISP1Path::init(MediaDevice *media)
>                 return false;
>  
>         populateFormats();
> +       ispMaxInSize_ = ispMaxInSize;
>  
>         link_ = media->link("rkisp1_isp", 2, resizer, 0);
>         if (!link_)
> @@ -172,7 +173,9 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,
>                 /* Add all the RAW sizes the sensor can produce for this code. */
>                 for (const auto &rawSize : sensor->sizes(mbusCode)) {
>                         if (rawSize.width > maxResolution_.width ||
> -                           rawSize.height > maxResolution_.height)
> +                           rawSize.height > maxResolution_.height ||
> +                           rawSize.width > ispMaxInSize_.width ||
> +                           rawSize.height > ispMaxInSize_.height)
>                                 continue;
>  
>                         streamFormats[format].push_back({ rawSize, rawSize });
> @@ -275,8 +278,9 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
>         if (!found)
>                 cfg->pixelFormat = isRaw ? rawFormat : formats::NV12;
>  
> -       Size minResolution;
> -       Size maxResolution;
> +       Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> +                                          .boundedTo(resolution);
> +       Size minResolution = minResolution_.expandedToAspectRatio(resolution);
>  
>         if (isRaw) {
>                 /*
> @@ -287,16 +291,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(ispMaxInSize_);
> +                       maxResolution = sensorFormat.size.boundedTo(ispMaxInSize_);
> +               }
>         }
>  
>         cfg->size.boundTo(maxResolution);
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> index cd77957ee..c96bd5557 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 &ispMaxInSize);
>  
>         int setEnabled(bool enable) { return link_->setEnabled(enable); }
>         bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }
> @@ -77,6 +77,8 @@ private:
>         std::unique_ptr<V4L2Subdevice> resizer_;
>         std::unique_ptr<V4L2VideoDevice> video_;
>         MediaLink *link_;
> +
> +       Size ispMaxInSize_;
>  };
>  
>  class RkISP1MainPath : public RkISP1Path
> -- 
> 2.39.2
>

On Wed, Jan 17, 2024 at 11:43:43AM +0000, Kieran Bingham via libcamera-devel wrote:
> Quoting Paul Elder via libcamera-devel (2024-01-16 09:17:53)
> > 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 clamping the sensor size to the ISP
> > input size at validation time.
> 
> I think we should do more here (in a separate patch) to filter out
> sensor modes that are larger than the ISP max as well.
> 
> At the moment it seems too easy for the rkisp1 pipeline handler to
> select an input configuration that it can't actually configure.
> 
> But that's aside from this patch itself.
> 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 17 ++++++++++--
> >  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 26 +++++++++----------
> >  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  4 ++-
> >  3 files changed, 30 insertions(+), 17 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 586b46d64..fb67ba8f4 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 ISP maximum input size for config validation in the path

s/Retrieve/Retrieve the/

> > +        * classes

s/$/./

> > +        */
> > +       Size ispMaxInSize = { 0, 0 };

That's the default, so you can just write

	Size ispMaxInSize;

I also find ispMaxInputSize more readable.

> > +       V4L2Subdevice::Formats ispFormats = isp_->formats(0);

const if you don't need to modify it.

> > +       for (const auto &[mbus, sizes] : ispFormats) {
> > +               for (const auto &size : sizes) {
> > +                       if (ispMaxInSize < size.max)
> > +                               ispMaxInSize = size.max;
> > +               }
> > +       }

Do the maximum input size depend on the media bus format ? And to we
have multiple sizes in the sizes vector ? We could take some shortcuts,
knowing how the ISP driver is implemented, taking the first entry in
both ispFormats and sizes.

> > +
> >         /* Locate and open the ISP main and self paths. */
> > -       if (!mainPath_.init(media_))
> > +       if (!mainPath_.init(media_, ispMaxInSize))
> >                 return false;
> >  
> > -       if (hasSelfPath_ && !selfPath_.init(media_))
> > +       if (hasSelfPath_ && !selfPath_.init(media_, ispMaxInSize))
> >                 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 b62b645ca..eaab7e857 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 &ispMaxInSize)

ispMaxInputSize here too if you rename it above. Same below.

> It feels a bit odd to me passing ispMaxInSize as the only parameter to
> initialize. But I guess it save init from re-determining it for each
> path, and maybe it's the only variable for now - so I think we're ok
> here.
> 
> I wonder if we should iterate and 'try' configurations to only expose
> those which can be supported. But we can tackle that on top I think.

Agreed.

> So for now I am ok with this:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

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

with the above comments taken into account.

> 
> >  {
> >         std::string resizer = std::string("rkisp1_resizer_") + name_ + "path";
> >         std::string video = std::string("rkisp1_") + name_ + "path";
> > @@ -76,6 +76,7 @@ bool RkISP1Path::init(MediaDevice *media)
> >                 return false;
> >  
> >         populateFormats();
> > +       ispMaxInSize_ = ispMaxInSize;
> >  
> >         link_ = media->link("rkisp1_isp", 2, resizer, 0);
> >         if (!link_)
> > @@ -172,7 +173,9 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,
> >                 /* Add all the RAW sizes the sensor can produce for this code. */
> >                 for (const auto &rawSize : sensor->sizes(mbusCode)) {
> >                         if (rawSize.width > maxResolution_.width ||
> > -                           rawSize.height > maxResolution_.height)
> > +                           rawSize.height > maxResolution_.height ||
> > +                           rawSize.width > ispMaxInSize_.width ||
> > +                           rawSize.height > ispMaxInSize_.height)
> >                                 continue;
> >  
> >                         streamFormats[format].push_back({ rawSize, rawSize });
> > @@ -275,8 +278,9 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
> >         if (!found)
> >                 cfg->pixelFormat = isRaw ? rawFormat : formats::NV12;
> >  
> > -       Size minResolution;
> > -       Size maxResolution;
> > +       Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> > +                                          .boundedTo(resolution);
> > +       Size minResolution = minResolution_.expandedToAspectRatio(resolution);
> >  
> >         if (isRaw) {
> >                 /*
> > @@ -287,16 +291,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(ispMaxInSize_);
> > +                       maxResolution = sensorFormat.size.boundedTo(ispMaxInSize_);
> > +               }
> >         }
> >  
> >         cfg->size.boundTo(maxResolution);
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > index cd77957ee..c96bd5557 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 &ispMaxInSize);
> >  
> >         int setEnabled(bool enable) { return link_->setEnabled(enable); }
> >         bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }
> > @@ -77,6 +77,8 @@ private:
> >         std::unique_ptr<V4L2Subdevice> resizer_;
> >         std::unique_ptr<V4L2VideoDevice> video_;
> >         MediaLink *link_;
> > +
> > +       Size ispMaxInSize_;
> >  };
> >  
> >  class RkISP1MainPath : public RkISP1Path

Hi Paul

On Tue, Jan 16, 2024 at 06:17:53PM +0900, Paul Elder via libcamera-devel wrote:
> 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 clamping the sensor size to the ISP
> input size at validation time.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 17 ++++++++++--
>  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 26 +++++++++----------
>  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  4 ++-
>  3 files changed, 30 insertions(+), 17 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 586b46d64..fb67ba8f4 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 ISP maximum input size for config validation in the path
> +	 * classes
> +	 */
> +	Size ispMaxInSize = { 0, 0 };
> +	V4L2Subdevice::Formats ispFormats = isp_->formats(0);
> +	for (const auto &[mbus, sizes] : ispFormats) {
> +		for (const auto &size : sizes) {
> +			if (ispMaxInSize < size.max)
> +				ispMaxInSize = size.max;
> +		}
> +	}
> +

I presume this could even be kept as a constant in the pipeline
handler ? Sure, retrieving it a run time is probably better

>  	/* Locate and open the ISP main and self paths. */
> -	if (!mainPath_.init(media_))
> +	if (!mainPath_.init(media_, ispMaxInSize))
>  		return false;
>
> -	if (hasSelfPath_ && !selfPath_.init(media_))
> +	if (hasSelfPath_ && !selfPath_.init(media_, ispMaxInSize))
>  		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 b62b645ca..eaab7e857 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 &ispMaxInSize)
>  {
>  	std::string resizer = std::string("rkisp1_resizer_") + name_ + "path";
>  	std::string video = std::string("rkisp1_") + name_ + "path";
> @@ -76,6 +76,7 @@ bool RkISP1Path::init(MediaDevice *media)
>  		return false;
>
>  	populateFormats();
> +	ispMaxInSize_ = ispMaxInSize;
>
>  	link_ = media->link("rkisp1_isp", 2, resizer, 0);
>  	if (!link_)
> @@ -172,7 +173,9 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,
>  		/* Add all the RAW sizes the sensor can produce for this code. */
>  		for (const auto &rawSize : sensor->sizes(mbusCode)) {
>  			if (rawSize.width > maxResolution_.width ||
> -			    rawSize.height > maxResolution_.height)
> +			    rawSize.height > maxResolution_.height ||
> +			    rawSize.width > ispMaxInSize_.width ||
> +			    rawSize.height > ispMaxInSize_.height)

This filters out the streamFormats that are reported to the
application, and I think this should be maybe only limited by the
video capture device output sizes, not the isp input sizes ?

I do however see a few lines above in generateConfiguration()

	const Size &resolution = sensor->resolution();

	/* Min and max resolutions to populate the available stream formats. */
	Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
					   .boundedTo(resolution);
	Size minResolution = minResolution_.expandedToAspectRatio(resolution);

Should `resolution` be replaced by the maximum sensor resolution
smaller than the isp max input size (maybe computed in
populateFormats() ? )


>  				continue;
>
>  			streamFormats[format].push_back({ rawSize, rawSize });
> @@ -275,8 +278,9 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
>  	if (!found)
>  		cfg->pixelFormat = isRaw ? rawFormat : formats::NV12;
>
> -	Size minResolution;
> -	Size maxResolution;
> +	Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> +					   .boundedTo(resolution);
> +	Size minResolution = minResolution_.expandedToAspectRatio(resolution);

Should you replace 'resolution' with the above mentioned 'larger
sensor's resolution which is smaller than the ISP input size' ?

>
>  	if (isRaw) {
>  		/*
> @@ -287,16 +291,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(ispMaxInSize_);
> +			maxResolution = sensorFormat.size.boundedTo(ispMaxInSize_);
> +		}
>  	}
>
>  	cfg->size.boundTo(maxResolution);
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> index cd77957ee..c96bd5557 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 &ispMaxInSize);

I would creat the path with a pointer to the isp subdev instead of
passing the value here as this doesn't smell like a function parameter
but a system's characterstics . Not a big deal though.

>
>  	int setEnabled(bool enable) { return link_->setEnabled(enable); }
>  	bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }
> @@ -77,6 +77,8 @@ private:
>  	std::unique_ptr<V4L2Subdevice> resizer_;
>  	std::unique_ptr<V4L2VideoDevice> video_;
>  	MediaLink *link_;
> +
> +	Size ispMaxInSize_;
>  };
>
>  class RkISP1MainPath : public RkISP1Path
> --
> 2.39.2
>