| Message ID | 20220719121003.1829916-3-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thank you for the patch. On Tue, Jul 19, 2022 at 09:10:03PM +0900, Paul Elder via libcamera-devel wrote: > Query the driver for the output formats that it supports, instead of > hardcoding it. > > This allows future-proofing for formats that are supported by some but > not all versions of the driver. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > One problem with this patch is that it causes YVU422 to be reported as > supported, but as discussed before, libcamera will try to configure a > non-existent pixelformat to the V4L2 device, as YVU422P does not exist > as a V4L2 format, and there currently is no infrastructure in place to > let libcamera configure YVU422M instead (which would work). I think that's fine for now, planar YUV formats are less commonly used that packed and semi-planar YUV formats, so we can live with this until Jacopo's work gets merged. > A patch [1] was submitted to fix this (tell libcamera to configure > YVU422M instead of the non-existent YVU422P), but it has been canceled > as a parallel effort is apparently in place [2]. > > [1] https://patchwork.libcamera.org/patch/16573/ > [2] https://patchwork.libcamera.org/patch/16573/#23799 > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 50 +++++++++---------- > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 6 ++- > 2 files changed, 29 insertions(+), 27 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > index 6f175758..00b5c3ed 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > @@ -20,9 +20,9 @@ namespace libcamera { > > LOG_DECLARE_CATEGORY(RkISP1) > > -RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats, > +RkISP1Path::RkISP1Path(const char *name, > const Size &minResolution, const Size &maxResolution) > - : name_(name), running_(false), formats_(formats), > + : name_(name), running_(false), > minResolution_(minResolution), maxResolution_(maxResolution), > link_(nullptr) > { > @@ -41,6 +41,8 @@ bool RkISP1Path::init(MediaDevice *media) > if (video_->open() < 0) > return false; > > + formats_ = fetchFormats(video_); if (formats_.empty()) /* Error handling */ > + > link_ = media->link("rkisp1_isp", 2, resizer, 0); > if (!link_) > return false; > @@ -48,6 +50,24 @@ bool RkISP1Path::init(MediaDevice *media) > return true; > } > > +std::vector<PixelFormat> RkISP1Path::fetchFormats(std::unique_ptr<V4L2VideoDevice> &video) You could call this populateFormats() and operate on formats_ directly, instead of returning a vector. This would free the return value for an error code, and will also come handy (I think) when expanding this function to also populate the min/max supported sizes dynamically. > +{ > + V4L2VideoDevice::Formats v4l2Formats = video->formats(0, true); > + std::vector<PixelFormat> formats; > + > + for (auto pair : v4l2Formats) { > + const PixelFormat pixelFormat = pair.first.toPixelFormat(); > + const PixelFormatInfo info = PixelFormatInfo::info(pixelFormat); const PixelFormatInfo &info > + > + if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) > + continue; > + > + formats.push_back(pixelFormat); > + } > + > + return formats; > +} > + > StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution) > { > Size maxResolution = maxResolution_.boundedToAspectRatio(resolution) > @@ -72,6 +92,7 @@ CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg) > const StreamConfiguration reqCfg = *cfg; > CameraConfiguration::Status status = CameraConfiguration::Valid; > > + /* NV12 is definitely supported, no need to check */ I'd move this comment within the if, or write it as /* * Default to NV12 if the requested format is not supported. All * versions of the ISP are guaranteed to support NV12 on both the main * and self paths. */ > if (std::find(formats_.begin(), formats_.end(), cfg->pixelFormat) == > formats_.end()) > cfg->pixelFormat = formats::NV12; > @@ -207,39 +228,18 @@ void RkISP1Path::stop() > namespace { > constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 }; > constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 }; > -constexpr std::array<PixelFormat, 6> RKISP1_RSZ_MP_FORMATS{ > - formats::YUYV, > - formats::NV16, > - formats::NV61, > - formats::NV21, > - formats::NV12, > - formats::R8, > - /* \todo Add support for RAW formats. */ > -}; > > constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 }; > constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 }; > -constexpr std::array<PixelFormat, 8> RKISP1_RSZ_SP_FORMATS{ > - formats::YUYV, > - formats::NV16, > - formats::NV61, > - formats::NV21, > - formats::NV12, > - formats::R8, > - formats::RGB565, > - formats::XRGB8888, > -}; > } /* namespace */ > > RkISP1MainPath::RkISP1MainPath() > - : RkISP1Path("main", RKISP1_RSZ_MP_FORMATS, > - RKISP1_RSZ_MP_SRC_MIN, RKISP1_RSZ_MP_SRC_MAX) > + : RkISP1Path("main", RKISP1_RSZ_MP_SRC_MIN, RKISP1_RSZ_MP_SRC_MAX) > { > } > > RkISP1SelfPath::RkISP1SelfPath() > - : RkISP1Path("self", RKISP1_RSZ_SP_FORMATS, > - RKISP1_RSZ_SP_SRC_MIN, RKISP1_RSZ_SP_SRC_MAX) > + : RkISP1Path("self", RKISP1_RSZ_SP_SRC_MIN, RKISP1_RSZ_SP_SRC_MAX) I like how Jacopo's suggestion in 1/2 will allow dropping the min/max values passed to the constructor here. This could be done in this patch, or on top. > { > } > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > index f3f1ae39..77ea632b 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > @@ -30,7 +30,7 @@ struct V4L2SubdeviceFormat; > class RkISP1Path > { > public: > - RkISP1Path(const char *name, const Span<const PixelFormat> &formats, > + RkISP1Path(const char *name, > const Size &minResolution, const Size &maxResolution); > > bool init(MediaDevice *media); > @@ -57,12 +57,14 @@ public: > Signal<FrameBuffer *> &bufferReady() { return video_->bufferReady; } > > private: > + std::vector<PixelFormat> fetchFormats(std::unique_ptr<V4L2VideoDevice> &video); > + > static constexpr unsigned int RKISP1_BUFFER_COUNT = 4; > > const char *name_; > bool running_; > > - const Span<const PixelFormat> formats_; > + std::vector<PixelFormat> formats_; > const Size minResolution_; > const Size maxResolution_; >
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp index 6f175758..00b5c3ed 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp @@ -20,9 +20,9 @@ namespace libcamera { LOG_DECLARE_CATEGORY(RkISP1) -RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats, +RkISP1Path::RkISP1Path(const char *name, const Size &minResolution, const Size &maxResolution) - : name_(name), running_(false), formats_(formats), + : name_(name), running_(false), minResolution_(minResolution), maxResolution_(maxResolution), link_(nullptr) { @@ -41,6 +41,8 @@ bool RkISP1Path::init(MediaDevice *media) if (video_->open() < 0) return false; + formats_ = fetchFormats(video_); + link_ = media->link("rkisp1_isp", 2, resizer, 0); if (!link_) return false; @@ -48,6 +50,24 @@ bool RkISP1Path::init(MediaDevice *media) return true; } +std::vector<PixelFormat> RkISP1Path::fetchFormats(std::unique_ptr<V4L2VideoDevice> &video) +{ + V4L2VideoDevice::Formats v4l2Formats = video->formats(0, true); + std::vector<PixelFormat> formats; + + for (auto pair : v4l2Formats) { + const PixelFormat pixelFormat = pair.first.toPixelFormat(); + const PixelFormatInfo info = PixelFormatInfo::info(pixelFormat); + + if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) + continue; + + formats.push_back(pixelFormat); + } + + return formats; +} + StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution) { Size maxResolution = maxResolution_.boundedToAspectRatio(resolution) @@ -72,6 +92,7 @@ CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg) const StreamConfiguration reqCfg = *cfg; CameraConfiguration::Status status = CameraConfiguration::Valid; + /* NV12 is definitely supported, no need to check */ if (std::find(formats_.begin(), formats_.end(), cfg->pixelFormat) == formats_.end()) cfg->pixelFormat = formats::NV12; @@ -207,39 +228,18 @@ void RkISP1Path::stop() namespace { constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 }; constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 }; -constexpr std::array<PixelFormat, 6> RKISP1_RSZ_MP_FORMATS{ - formats::YUYV, - formats::NV16, - formats::NV61, - formats::NV21, - formats::NV12, - formats::R8, - /* \todo Add support for RAW formats. */ -}; constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 }; constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 }; -constexpr std::array<PixelFormat, 8> RKISP1_RSZ_SP_FORMATS{ - formats::YUYV, - formats::NV16, - formats::NV61, - formats::NV21, - formats::NV12, - formats::R8, - formats::RGB565, - formats::XRGB8888, -}; } /* namespace */ RkISP1MainPath::RkISP1MainPath() - : RkISP1Path("main", RKISP1_RSZ_MP_FORMATS, - RKISP1_RSZ_MP_SRC_MIN, RKISP1_RSZ_MP_SRC_MAX) + : RkISP1Path("main", RKISP1_RSZ_MP_SRC_MIN, RKISP1_RSZ_MP_SRC_MAX) { } RkISP1SelfPath::RkISP1SelfPath() - : RkISP1Path("self", RKISP1_RSZ_SP_FORMATS, - RKISP1_RSZ_SP_SRC_MIN, RKISP1_RSZ_SP_SRC_MAX) + : RkISP1Path("self", RKISP1_RSZ_SP_SRC_MIN, RKISP1_RSZ_SP_SRC_MAX) { } diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h index f3f1ae39..77ea632b 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h @@ -30,7 +30,7 @@ struct V4L2SubdeviceFormat; class RkISP1Path { public: - RkISP1Path(const char *name, const Span<const PixelFormat> &formats, + RkISP1Path(const char *name, const Size &minResolution, const Size &maxResolution); bool init(MediaDevice *media); @@ -57,12 +57,14 @@ public: Signal<FrameBuffer *> &bufferReady() { return video_->bufferReady; } private: + std::vector<PixelFormat> fetchFormats(std::unique_ptr<V4L2VideoDevice> &video); + static constexpr unsigned int RKISP1_BUFFER_COUNT = 4; const char *name_; bool running_; - const Span<const PixelFormat> formats_; + std::vector<PixelFormat> formats_; const Size minResolution_; const Size maxResolution_;
Query the driver for the output formats that it supports, instead of hardcoding it. This allows future-proofing for formats that are supported by some but not all versions of the driver. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- One problem with this patch is that it causes YVU422 to be reported as supported, but as discussed before, libcamera will try to configure a non-existent pixelformat to the V4L2 device, as YVU422P does not exist as a V4L2 format, and there currently is no infrastructure in place to let libcamera configure YVU422M instead (which would work). A patch [1] was submitted to fix this (tell libcamera to configure YVU422M instead of the non-existent YVU422P), but it has been canceled as a parallel effort is apparently in place [2]. [1] https://patchwork.libcamera.org/patch/16573/ [2] https://patchwork.libcamera.org/patch/16573/#23799 src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 50 +++++++++---------- src/libcamera/pipeline/rkisp1/rkisp1_path.h | 6 ++- 2 files changed, 29 insertions(+), 27 deletions(-)