[libcamera-devel] [PATCH v3 15/22] libcamera: pipeline: rkisp1: Breakout basic path handling to own class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Sep 29 00:12:31 CEST 2020
Hi Niklas,
Thank you for the patch.
On Fri, Sep 25, 2020 at 03:42:00AM +0200, Niklas Söderlund wrote:
> The self and main paths are very similar and the introduction of support
> for two paths simulating sly (paths) have made it clear their handling
Are you using voice recognition software ? :-)
> could be abstracted in a separate class.
>
> This is the first step to create such an class by breaking out the
s/an class/a class/
> initialization and storage of the video and subdevices. There is no
> functional change in this patch.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> src/libcamera/pipeline/rkisp1/meson.build | 1 +
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 104 ++++++++-----------
> src/libcamera/pipeline/rkisp1/rkisp1path.cpp | 53 ++++++++++
> src/libcamera/pipeline/rkisp1/rkisp1path.h | 46 ++++++++
> 4 files changed, 144 insertions(+), 60 deletions(-)
> create mode 100644 src/libcamera/pipeline/rkisp1/rkisp1path.cpp
> create mode 100644 src/libcamera/pipeline/rkisp1/rkisp1path.h
>
> diff --git a/src/libcamera/pipeline/rkisp1/meson.build b/src/libcamera/pipeline/rkisp1/meson.build
> index 1ab3964a6db190f0..eddf795e54575956 100644
> --- a/src/libcamera/pipeline/rkisp1/meson.build
> +++ b/src/libcamera/pipeline/rkisp1/meson.build
> @@ -2,5 +2,6 @@
>
> libcamera_sources += files([
> 'rkisp1.cpp',
> + 'rkisp1path.cpp',
Maybe rkisp1_path.cpp ?
> 'timeline.cpp',
> ])
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 27a3c44da3805c5f..e738a7eb19264d79 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -33,6 +33,7 @@
> #include "libcamera/internal/v4l2_subdevice.h"
> #include "libcamera/internal/v4l2_videodevice.h"
>
> +#include "rkisp1path.h"
> #include "timeline.h"
>
> namespace libcamera {
> @@ -147,12 +148,11 @@ public:
> class RkISP1CameraData : public CameraData
> {
> public:
> - RkISP1CameraData(PipelineHandler *pipe, V4L2VideoDevice *mainPathVideo,
> - V4L2VideoDevice *selfPathVideo)
> + RkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath,
> + RkISP1SelfPath *selfPath)
> : CameraData(pipe), sensor_(nullptr), frame_(0),
> - frameInfo_(pipe), mainPathVideo_(mainPathVideo),
> - selfPathVideo_(selfPathVideo), mainPathActive_(false),
> - selfPathActive_(false)
> + frameInfo_(pipe), mainPath_(mainPath), selfPath_(selfPath),
> + mainPathActive_(false), selfPathActive_(false)
> {
> }
>
> @@ -171,8 +171,8 @@ public:
> RkISP1Frames frameInfo_;
> RkISP1Timeline timeline_;
>
> - V4L2VideoDevice *mainPathVideo_;
> - V4L2VideoDevice *selfPathVideo_;
> + RkISP1MainPath *mainPath_;
> + RkISP1SelfPath *selfPath_;
>
> bool mainPathActive_;
> bool selfPathActive_;
> @@ -259,13 +259,12 @@ private:
>
> MediaDevice *media_;
> V4L2Subdevice *isp_;
> - V4L2Subdevice *mainPathResizer_;
> - V4L2Subdevice *selfPathResizer_;
> - V4L2VideoDevice *mainPathVideo_;
> - V4L2VideoDevice *selfPathVideo_;
> V4L2VideoDevice *param_;
> V4L2VideoDevice *stat_;
>
> + RkISP1MainPath mainPath_;
> + RkISP1SelfPath selfPath_;
> +
> std::vector<std::unique_ptr<FrameBuffer>> paramBuffers_;
> std::vector<std::unique_ptr<FrameBuffer>> statBuffers_;
> std::queue<FrameBuffer *> availableParamBuffers_;
> @@ -441,10 +440,10 @@ protected:
> pipe_->stat_->queueBuffer(info->statBuffer);
>
> if (info->mainPathBuffer)
> - pipe_->mainPathVideo_->queueBuffer(info->mainPathBuffer);
> + pipe_->mainPath_.video_->queueBuffer(info->mainPathBuffer);
>
> if (info->selfPathBuffer)
> - pipe_->selfPathVideo_->queueBuffer(info->selfPathBuffer);
> + pipe_->selfPath_.video_->queueBuffer(info->selfPathBuffer);
> }
>
> private:
> @@ -554,13 +553,13 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(
> CameraConfiguration::Status RkISP1CameraConfiguration::validateMainPath(StreamConfiguration *cfg)
> {
> return validatePath(cfg, RKISP1_RSZ_MP_FORMATS, RKISP1_RSZ_MP_SRC_MIN,
> - RKISP1_RSZ_MP_SRC_MAX, data_->mainPathVideo_);
> + RKISP1_RSZ_MP_SRC_MAX, data_->mainPath_->video_);
> }
>
> CameraConfiguration::Status RkISP1CameraConfiguration::validateSelfPath(StreamConfiguration *cfg)
> {
> return validatePath(cfg, RKISP1_RSZ_SP_FORMATS, RKISP1_RSZ_SP_SRC_MIN,
> - RKISP1_RSZ_SP_SRC_MAX, data_->selfPathVideo_);
> + RKISP1_RSZ_SP_SRC_MAX, data_->selfPath_->video_);
> }
>
> bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)
> @@ -688,9 +687,8 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> }
>
> PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
> - : PipelineHandler(manager), isp_(nullptr), mainPathResizer_(nullptr),
> - selfPathResizer_(nullptr), mainPathVideo_(nullptr),
> - selfPathVideo_(nullptr), param_(nullptr), stat_(nullptr)
> + : PipelineHandler(manager), isp_(nullptr), param_(nullptr),
> + stat_(nullptr)
> {
> }
>
> @@ -698,10 +696,6 @@ PipelineHandlerRkISP1::~PipelineHandlerRkISP1()
> {
> delete param_;
> delete stat_;
> - delete mainPathVideo_;
> - delete selfPathVideo_;
> - delete mainPathResizer_;
> - delete selfPathResizer_;
> delete isp_;
> }
>
> @@ -821,12 +815,12 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> V4L2VideoDevice *video;
>
> if (cfg.stream() == &data->mainPathStream_) {
> - resizer = mainPathResizer_;
> - video = mainPathVideo_;
> + resizer = mainPath_.resizer_;
> + video = mainPath_.video_;
> data->mainPathActive_ = true;
> } else {
> - resizer = selfPathResizer_;
> - video = selfPathVideo_;
> + resizer = selfPath_.resizer_;
> + video = selfPath_.video_;
> data->selfPathActive_ = true;
> }
>
> @@ -834,7 +828,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> if (ret < 0)
> return ret;
>
> - const char *name = resizer == mainPathResizer_ ? "main" : "self";
> + const char *name = resizer == mainPath_.resizer_ ? "main" : "self";
>
> LOG(RkISP1, Debug)
> << "Configured " << name << " resizer input pad with "
> @@ -894,9 +888,9 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S
> unsigned int count = stream->configuration().bufferCount;
>
> if (stream == &data->mainPathStream_)
> - return mainPathVideo_->exportBuffers(count, buffers);
> + return mainPath_.video_->exportBuffers(count, buffers);
> else if (stream == &data->selfPathStream_)
> - return selfPathVideo_->exportBuffers(count, buffers);
> + return selfPath_.video_->exportBuffers(count, buffers);
>
> return -EINVAL;
> }
> @@ -913,14 +907,14 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
> });
>
> if (data->mainPathActive_) {
> - ret = mainPathVideo_->importBuffers(
> + ret = mainPath_.video_->importBuffers(
> data->mainPathStream_.configuration().bufferCount);
> if (ret < 0)
> goto error;
> }
>
> if (data->selfPathActive_) {
> - ret = selfPathVideo_->importBuffers(
> + ret = selfPath_.video_->importBuffers(
> data->selfPathStream_.configuration().bufferCount);
> if (ret < 0)
> goto error;
> @@ -955,8 +949,8 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
> error:
> paramBuffers_.clear();
> statBuffers_.clear();
> - mainPathVideo_->releaseBuffers();
> - selfPathVideo_->releaseBuffers();
> + mainPath_.video_->releaseBuffers();
> + selfPath_.video_->releaseBuffers();
>
> return ret;
> }
> @@ -987,10 +981,10 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)
> if (stat_->releaseBuffers())
> LOG(RkISP1, Error) << "Failed to release stat buffers";
>
> - if (mainPathVideo_->releaseBuffers())
> + if (mainPath_.video_->releaseBuffers())
> LOG(RkISP1, Error) << "Failed to release main path buffers";
>
> - if (selfPathVideo_->releaseBuffers())
> + if (selfPath_.video_->releaseBuffers())
> LOG(RkISP1, Error) << "Failed to release self path buffers";
>
> return 0;
> @@ -1038,7 +1032,7 @@ int PipelineHandlerRkISP1::start(Camera *camera)
> std::map<unsigned int, IPAStream> streamConfig;
>
> if (data->mainPathActive_) {
> - ret = mainPathVideo_->streamOn();
> + ret = mainPath_.video_->streamOn();
> if (ret) {
> param_->streamOff();
> stat_->streamOff();
> @@ -1057,10 +1051,10 @@ int PipelineHandlerRkISP1::start(Camera *camera)
> }
>
> if (data->selfPathActive_) {
> - ret = selfPathVideo_->streamOn();
> + ret = selfPath_.video_->streamOn();
> if (ret) {
> if (data->mainPathActive_)
> - mainPathVideo_->streamOff();
> + mainPath_.video_->streamOff();
>
> param_->streamOff();
> stat_->streamOff();
> @@ -1106,7 +1100,7 @@ void PipelineHandlerRkISP1::stop(Camera *camera)
> int ret;
>
> if (data->selfPathActive_) {
> - ret = selfPathVideo_->streamOff();
> + ret = selfPath_.video_->streamOff();
> if (ret)
> LOG(RkISP1, Warning)
> << "Failed to stop self path for "
> @@ -1114,7 +1108,7 @@ void PipelineHandlerRkISP1::stop(Camera *camera)
> }
>
> if (data->mainPathActive_) {
> - ret = mainPathVideo_->streamOff();
> + ret = mainPath_.video_->streamOff();
> if (ret)
> LOG(RkISP1, Warning)
> << "Failed to stop main path for "
> @@ -1226,8 +1220,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> int ret;
>
> std::unique_ptr<RkISP1CameraData> data =
> - std::make_unique<RkISP1CameraData>(this, mainPathVideo_,
> - selfPathVideo_);
> + std::make_unique<RkISP1CameraData>(this, &mainPath_, &selfPath_);
>
> ControlInfoMap::Map ctrls;
> ctrls.emplace(std::piecewise_construct,
> @@ -1281,23 +1274,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
> if (isp_->open() < 0)
> return false;
>
> - mainPathResizer_ = V4L2Subdevice::fromEntityName(media_, "rkisp1_resizer_mainpath");
> - if (mainPathResizer_->open() < 0)
> - return false;
> -
> - selfPathResizer_ = V4L2Subdevice::fromEntityName(media_, "rkisp1_resizer_selfpath");
> - if (selfPathResizer_->open() < 0)
> - return false;
> -
> /* Locate and open the capture video node. */
This comment seems outdated.
> - mainPathVideo_ = V4L2VideoDevice::fromEntityName(media_, "rkisp1_mainpath");
> - if (mainPathVideo_->open() < 0)
> - return false;
> -
> - selfPathVideo_ = V4L2VideoDevice::fromEntityName(media_, "rkisp1_selfpath");
> - if (selfPathVideo_->open() < 0)
> - return false;
> -
> stat_ = V4L2VideoDevice::fromEntityName(media_, "rkisp1_stats");
> if (stat_->open() < 0)
> return false;
> @@ -1306,8 +1283,15 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
> if (param_->open() < 0)
> return false;
>
> - mainPathVideo_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);
> - selfPathVideo_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);
> + /* Locate and open the ISP main and self paths. */
> + if (!mainPath_.init(media_))
> + return false;
> +
> + if (!selfPath_.init(media_))
> + return false;
> +
> + mainPath_.video_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);
> + selfPath_.video_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);
> stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);
> param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1path.cpp
> new file mode 100644
> index 0000000000000000..51a75df86baaaa7b
> --- /dev/null
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1path.cpp
> @@ -0,0 +1,53 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * rkisp1path.cpp - Rockchip ISP1 path helper
> + */
> +
> +#include "rkisp1path.h"
> +
> +#include "libcamera/internal/media_device.h"
> +#include "libcamera/internal/v4l2_subdevice.h"
> +#include "libcamera/internal/v4l2_videodevice.h"
> +
> +namespace libcamera {
> +
> +RkISP1Path::RkISP1Path(const char *name)
> + : resizer_(nullptr), video_(nullptr), name_(name)
> +{
> +}
> +
> +RkISP1Path::~RkISP1Path()
> +{
> + delete video_;
> + delete resizer_;
Should these two be stored in std::unique_ptr<> ? It would make sense
for V4L2Subdevice::fromEntityName() to return a unique pointer actually
(but that can be addressed separately from this series).
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> +}
> +
> +bool RkISP1Path::init(MediaDevice *media)
> +{
> + std::string resizer = std::string("rkisp1_resizer_") + name_ + "path";
> + std::string video = std::string("rkisp1_") + name_ + "path";
> +
> + resizer_ = V4L2Subdevice::fromEntityName(media, resizer);
> + if (resizer_->open() < 0)
> + return false;
> +
> + video_ = V4L2VideoDevice::fromEntityName(media, video);
> + if (video_->open() < 0)
> + return false;
> +
> + return true;
> +}
> +
> +RkISP1MainPath::RkISP1MainPath()
> + : RkISP1Path("main")
> +{
> +}
> +
> +RkISP1SelfPath::RkISP1SelfPath()
> + : RkISP1Path("self")
> +{
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1path.h b/src/libcamera/pipeline/rkisp1/rkisp1path.h
> new file mode 100644
> index 0000000000000000..d3172e228a3f67bf
> --- /dev/null
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1path.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * rkisp1path.h - Rockchip ISP1 path helper
> + */
> +#ifndef __LIBCAMERA_PIPELINE_RKISP1_PATH_H__
> +#define __LIBCAMERA_PIPELINE_RKISP1_PATH_H__
> +
> +namespace libcamera {
> +
> +class MediaDevice;
> +class V4L2Subdevice;
> +class V4L2VideoDevice;
> +
> +class RkISP1Path
> +{
> +public:
> + RkISP1Path(const char *name);
> + ~RkISP1Path();
> +
> + bool init(MediaDevice *media);
> +
> + /* \todo Make resizer and video private. */
> + V4L2Subdevice *resizer_;
> + V4L2VideoDevice *video_;
> +
> +private:
> + const char *name_;
> +};
> +
> +class RkISP1MainPath : public RkISP1Path
> +{
> +public:
> + RkISP1MainPath();
> +};
> +
> +class RkISP1SelfPath : public RkISP1Path
> +{
> +public:
> + RkISP1SelfPath();
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_PIPELINE_RKISP1_PATH_H__ */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list