| Message ID | 20240909163719.267211-2-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Umang Jain (2024-09-09 17:37:18) > The RkISP1Path class has maximum resolution defined by > RKISP1_RSZ_*_SRC_MAX macros. It might get updated in populateFormats() > by querying the formats on the rkisp1_*path video nodes. However, it > does not take into account the maximum resolution that the ISP can > handle. > > For instance on i.MX8MP, 4096x3072 is the maximum supported ISP > resolution however, RkISP1MainPath had the maximum resolution of > RKISP1_RSZ_MP_SRC_MAX, prior to this patch. Does this mean that the kernel is reporting the wrong maximum sizes on the path video node? Seems like this should be a kernel side fix too ( but the fact that there are kernels that do not report the right size, means at least we need a libcamera side fix here now anyway). > > Fix it by bounding the maximum resolution of RkISP1Path class by passing > the maximum resolution of the ISP during RkISP1Path::init(). > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 +++++++++++++-- > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++- > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 +- > 3 files changed, 17 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 0a794d63..97ce8457 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -1274,6 +1274,17 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) > if (isp_->open() < 0) > return false; > > + /* > + * Retrieve the ISP maximum input size for config validation in the > + * path classes. > + * > + * The ISP maximum input size is independent of the media bus formats > + * hence, pick the one first entry of ispFormats and its size range. > + */ > + V4L2Subdevice::Formats ispFormats = isp_->formats(0); > + const SizeRange range = ispFormats.cbegin()->second[0]; > + Size ispMaxInputSize = range.max; > + > /* Locate and open the optional CSI-2 receiver. */ > ispSink_ = isp_->entity()->getPadByIndex(0); > if (!ispSink_ || ispSink_->links().empty()) > @@ -1300,10 +1311,10 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) > return false; > > /* 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..9053af18 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, Size ispMaxInputSize) > { > std::string resizer = std::string("rkisp1_resizer_") + name_ + "path"; > std::string video = std::string("rkisp1_") + name_ + "path"; > @@ -77,6 +77,8 @@ bool RkISP1Path::init(MediaDevice *media) > > populateFormats(); > > + maxResolution_.boundTo(ispMaxInputSize); > + I think this really needs a comment to explain... /* * The maximum size reported by the video node during populate * formats is hard coded on some kernels to a fixed size which * can exceed the platform specific ISP limitations. * * While this should be fixed in the kernel, restrict to the ISP * limitations correctly. */ or such ? > link_ = media->link("rkisp1_isp", 2, resizer, 0); > if (!link_) > return false; > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > index 08edefec..13ba4b62 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, Size ispMaxInputSize); I hoped this would be something we could set during the constructor - but it would require changing the allocations and constructs of those and it's probably not worth it for now so I could live with this. With comments to report /why/ we are clamping the sizes: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > int setEnabled(bool enable) { return link_->setEnabled(enable); } > bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; } > -- > 2.45.0 >
On Wed, Sep 11, 2024 at 12:30:52PM +0100, Kieran Bingham wrote: > Quoting Umang Jain (2024-09-09 17:37:18) > > The RkISP1Path class has maximum resolution defined by > > RKISP1_RSZ_*_SRC_MAX macros. It might get updated in populateFormats() > > by querying the formats on the rkisp1_*path video nodes. However, it > > does not take into account the maximum resolution that the ISP can > > handle. > > > > For instance on i.MX8MP, 4096x3072 is the maximum supported ISP > > resolution however, RkISP1MainPath had the maximum resolution of > > RKISP1_RSZ_MP_SRC_MAX, prior to this patch. > > Does this mean that the kernel is reporting the wrong maximum sizes on > the path video node? Isn't it the other way around, with RKISP1_RSZ_MP_SRC_MAX being wrong, and the kernel reporting the right value when querying the video node ? Different ISP versions have different limits, so we can't hardcode a single one. We could hardcode different limits for different versions in libcamera, but querying the limit dynamically is probably better. > Seems like this should be a kernel side fix too ( but the fact that > there are kernels that do not report the right size, means at least we > need a libcamera side fix here now anyway). > > > Fix it by bounding the maximum resolution of RkISP1Path class by passing > > the maximum resolution of the ISP during RkISP1Path::init(). > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > --- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 +++++++++++++-- > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++- > > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 +- > > 3 files changed, 17 insertions(+), 4 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 0a794d63..97ce8457 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -1274,6 +1274,17 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) > > if (isp_->open() < 0) > > return false; > > > > + /* > > + * Retrieve the ISP maximum input size for config validation in the > > + * path classes. > > + * > > + * The ISP maximum input size is independent of the media bus formats > > + * hence, pick the one first entry of ispFormats and its size range. > > + */ > > + V4L2Subdevice::Formats ispFormats = isp_->formats(0); const ? > > + const SizeRange range = ispFormats.cbegin()->second[0]; > > + Size ispMaxInputSize = range.max; const ? > > + > > /* Locate and open the optional CSI-2 receiver. */ > > ispSink_ = isp_->entity()->getPadByIndex(0); > > if (!ispSink_ || ispSink_->links().empty()) > > @@ -1300,10 +1311,10 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) > > return false; > > > > /* 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..9053af18 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, Size ispMaxInputSize) > > { > > std::string resizer = std::string("rkisp1_resizer_") + name_ + "path"; > > std::string video = std::string("rkisp1_") + name_ + "path"; > > @@ -77,6 +77,8 @@ bool RkISP1Path::init(MediaDevice *media) > > > > populateFormats(); > > > > + maxResolution_.boundTo(ispMaxInputSize); > > + > > I think this really needs a comment to explain... > > /* > * The maximum size reported by the video node during populate > * formats is hard coded on some kernels to a fixed size which > * can exceed the platform specific ISP limitations. > * > * While this should be fixed in the kernel, restrict to the ISP > * limitations correctly. I don't think that's right, see above. Did I miss something ? > */ > > or such ? > > > link_ = media->link("rkisp1_isp", 2, resizer, 0); > > if (!link_) > > return false; > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > > index 08edefec..13ba4b62 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, Size ispMaxInputSize); > > I hoped this would be something we could set during the constructor - > but it would require changing the allocations and constructs of those > and it's probably not worth it for now so I could live with this. > > With comments to report /why/ we are clamping the sizes: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > int setEnabled(bool enable) { return link_->setEnabled(enable); } > > bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }
Hi On 12/09/24 1:56 am, Laurent Pinchart wrote: > On Wed, Sep 11, 2024 at 12:30:52PM +0100, Kieran Bingham wrote: >> Quoting Umang Jain (2024-09-09 17:37:18) >>> The RkISP1Path class has maximum resolution defined by >>> RKISP1_RSZ_*_SRC_MAX macros. It might get updated in populateFormats() >>> by querying the formats on the rkisp1_*path video nodes. However, it >>> does not take into account the maximum resolution that the ISP can >>> handle. >>> >>> For instance on i.MX8MP, 4096x3072 is the maximum supported ISP >>> resolution however, RkISP1MainPath had the maximum resolution of >>> RKISP1_RSZ_MP_SRC_MAX, prior to this patch. >> Does this mean that the kernel is reporting the wrong maximum sizes on >> the path video node? > Isn't it the other way around, with RKISP1_RSZ_MP_SRC_MAX being wrong, > and the kernel reporting the right value when querying the video node ? RkISP1Path::populateFormats() does dynamic query of the sizes for the video node If the right values were reported from the video nodes, populateFormats() would have updated it, as it iterates over the sizes of the video node. > Different ISP versions have different limits, so we can't hardcode a > single one. We could hardcode different limits for different versions > in libcamera, but querying the limit dynamically is probably better. What I think is a missing link on the kernel side is the clamping of max width and height in rkisp1-capture.c with respect to ISP limits (specified in struct rkisp1_info) > >> Seems like this should be a kernel side fix too ( but the fact that >> there are kernels that do not report the right size, means at least we >> need a libcamera side fix here now anyway). Yes, indeed! >> >>> Fix it by bounding the maximum resolution of RkISP1Path class by passing >>> the maximum resolution of the ISP during RkISP1Path::init(). >>> >>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>> --- >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 +++++++++++++-- >>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++- >>> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 +- >>> 3 files changed, 17 insertions(+), 4 deletions(-) >>> >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >>> index 0a794d63..97ce8457 100644 >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >>> @@ -1274,6 +1274,17 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) >>> if (isp_->open() < 0) >>> return false; >>> >>> + /* >>> + * Retrieve the ISP maximum input size for config validation in the >>> + * path classes. >>> + * >>> + * The ISP maximum input size is independent of the media bus formats >>> + * hence, pick the one first entry of ispFormats and its size range. >>> + */ >>> + V4L2Subdevice::Formats ispFormats = isp_->formats(0); > const ? > >>> + const SizeRange range = ispFormats.cbegin()->second[0]; >>> + Size ispMaxInputSize = range.max; > const ? > >>> + >>> /* Locate and open the optional CSI-2 receiver. */ >>> ispSink_ = isp_->entity()->getPadByIndex(0); >>> if (!ispSink_ || ispSink_->links().empty()) >>> @@ -1300,10 +1311,10 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) >>> return false; >>> >>> /* 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..9053af18 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, Size ispMaxInputSize) >>> { >>> std::string resizer = std::string("rkisp1_resizer_") + name_ + "path"; >>> std::string video = std::string("rkisp1_") + name_ + "path"; >>> @@ -77,6 +77,8 @@ bool RkISP1Path::init(MediaDevice *media) >>> >>> populateFormats(); >>> >>> + maxResolution_.boundTo(ispMaxInputSize); >>> + >> I think this really needs a comment to explain... >> >> /* >> * The maximum size reported by the video node during populate >> * formats is hard coded on some kernels to a fixed size which >> * can exceed the platform specific ISP limitations. >> * >> * While this should be fixed in the kernel, restrict to the ISP >> * limitations correctly. > I don't think that's right, see above. Did I miss something ? I think Kieran's comment is correct. The ISP limits are model specific in the kernel as dictated by struct rkisp1_info. However, the resizer and path (rkisp1-capture.c) are using hard coded values RKISP1_RSZ_MP_SRC_MAX_WIDTH RKISP1_RSZ_MP_SRC_MAX_HEIGHT from drivers/media/platform/rockchip/rkisp1/rkisp1-common.h and that's what we in libcamera during populateFormats() as maxResolution_. > >> */ >> >> or such ? >> >>> link_ = media->link("rkisp1_isp", 2, resizer, 0); >>> if (!link_) >>> return false; >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h >>> index 08edefec..13ba4b62 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, Size ispMaxInputSize); >> I hoped this would be something we could set during the constructor - >> but it would require changing the allocations and constructs of those >> and it's probably not worth it for now so I could live with this. >> >> With comments to report /why/ we are clamping the sizes: >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >>> int setEnabled(bool enable) { return link_->setEnabled(enable); } >>> bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 0a794d63..97ce8457 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -1274,6 +1274,17 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) if (isp_->open() < 0) return false; + /* + * Retrieve the ISP maximum input size for config validation in the + * path classes. + * + * The ISP maximum input size is independent of the media bus formats + * hence, pick the one first entry of ispFormats and its size range. + */ + V4L2Subdevice::Formats ispFormats = isp_->formats(0); + const SizeRange range = ispFormats.cbegin()->second[0]; + Size ispMaxInputSize = range.max; + /* Locate and open the optional CSI-2 receiver. */ ispSink_ = isp_->entity()->getPadByIndex(0); if (!ispSink_ || ispSink_->links().empty()) @@ -1300,10 +1311,10 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) return false; /* 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..9053af18 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, Size ispMaxInputSize) { std::string resizer = std::string("rkisp1_resizer_") + name_ + "path"; std::string video = std::string("rkisp1_") + name_ + "path"; @@ -77,6 +77,8 @@ bool RkISP1Path::init(MediaDevice *media) populateFormats(); + maxResolution_.boundTo(ispMaxInputSize); + link_ = media->link("rkisp1_isp", 2, resizer, 0); if (!link_) return false; diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h index 08edefec..13ba4b62 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, Size ispMaxInputSize); int setEnabled(bool enable) { return link_->setEnabled(enable); } bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }
The RkISP1Path class has maximum resolution defined by RKISP1_RSZ_*_SRC_MAX macros. It might get updated in populateFormats() by querying the formats on the rkisp1_*path video nodes. However, it does not take into account the maximum resolution that the ISP can handle. For instance on i.MX8MP, 4096x3072 is the maximum supported ISP resolution however, RkISP1MainPath had the maximum resolution of RKISP1_RSZ_MP_SRC_MAX, prior to this patch. Fix it by bounding the maximum resolution of RkISP1Path class by passing the maximum resolution of the ISP during RkISP1Path::init(). Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 +++++++++++++-- src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++- src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 +- 3 files changed, 17 insertions(+), 4 deletions(-)