[libcamera-devel] [PATCH v3 15/22] libcamera: pipeline: rkisp1: Breakout basic path handling to own class
Niklas Söderlund
niklas.soderlund at ragnatech.se
Fri Sep 25 18:53:19 CEST 2020
Hi Jacopo,
Thanks for your feedback.
On 2020-09-25 17:12:03 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> wow a long subject!
>
> 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
>
> Is it me not knowing what "simulating sly (paths)" means ?
I think this is a secret my spellchecker will take to the grave :-)
The self and main paths are very similar and the introduction of
support for two simulations streams have made it clear their
handling...
Is what I intended to say.
>
> > could be abstracted in a separate class.
> >
> > This is the first step to create such an class by breaking out the
> > 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',
> > '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. */
> > - 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_;
> > +}
> > +
> > +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. */
>
> What is blocking this ?
Further work later in this series, fear not by 21/22 both are private
:-)
>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> Thanks
> j
>
> > + 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__ */
> > --
> > 2.28.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list