[libcamera-devel] [RFC PATCH 2/2] pipeline: rkisp1: Query the driver for formats
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jul 19 23:48:00 CEST 2022
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 at 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_;
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list