pipeline: rkisp1: Fix validation when sensor max is larger than ISP input

20699 diff mbox series

From: Paul Elder <paul.elder@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.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Umang Jain <umang.jain@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(-)

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@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@ideasonboard.com>
> Signed-off-by: Umang Jain <umang.jain@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

Hi Laurent

On 03/08/24 12:33 am, Laurent Pinchart wrote:
> 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@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@ideasonboard.com>
>> Signed-off-by: Umang Jain<umang.jain@ideasonboard.com>
>> ---
>> Split out fromhttps://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.

The goal here is to filter out sensor resolutions which cannot be 
supported on the ISP. And return the resolution which is equal to or 
just less than the max ISP input.

The notion was brought up in v1 of the patch here:
https://patchwork.libcamera.org/patch/19411/#28485

which  I agreed (it was never addressed until this patch).

```

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


For example, the platform I'm working with has imx283 and imx335 working 
with following sizes (in decreasing order of resolution):

IMX283: { 5472x3648, 4096x3072, 2736x1824 ...}
IMX335: { 2592x1944, ... }

So the function should filter out 5472x3648 for IMX283 which is higher 
than maxIspInputSize_ and return 4096x3072
For imx335, it returns 2592x1944 because that's the highest resolution 
here and the ISP shall support it.

>> +		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.

Ah, you are correct here. I will probably have to expand here by 
comparing the heights and widths individually.
>
>> +				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 ?

As I have explained briefly above, it varies with the sensor - similarly 
how sensor->resolution() varies which has been replaced with 
maxSupportedSensorResolution() here.
>> +	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 ?

Do you mean here main and self paths here? It's probably  be a guess at 
this time, I will have to check if there is some restriction when main 
and self paths are used simultaneously - and whether it affects the ISP 
input size.
> Overall this patch feels a bit too much of a hack for me to be
> comfortable with it.

Ok - I think I started out with v1 (had two R-b tags already) and only 
made improvement (maxSupportedSensorResolution()) which was also a 
review comment on the series.

If it feels like a hack - I'll drop this and probably need to approach 
the problem from a different angle here?

>
>> +	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