[libcamera-devel] [PATCH v3 15/22] libcamera: pipeline: rkisp1: Breakout basic path handling to own class
Jacopo Mondi
jacopo at jmondi.org
Mon Sep 28 10:40:22 CEST 2020
Hi Niklas,
On Fri, Sep 25, 2020 at 06:53:19PM +0200, Niklas Söderlund wrote:
> 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.
Maybe with
s/simulations/simultaneous ?
>
> >
> > > 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