[PATCH] pipeline: rkisp1: Fix validation when sensor max is larger than ISP input
Umang Jain
umang.jain at ideasonboard.com
Mon Aug 5 08:00:56 CEST 2024
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 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 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
More information about the libcamera-devel
mailing list