[libcamera-devel,v3,15/22] libcamera: pipeline: rkisp1: Breakout basic path handling to own class
diff mbox series

Message ID 20200925014207.1455796-16-niklas.soderlund@ragnatech.se
State Accepted
Headers show
Series
  • libcamera: pipeline: rkisp1: Extend to support two streams
Related show

Commit Message

Niklas Söderlund Sept. 25, 2020, 1:42 a.m. UTC 
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
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@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

Comments

Jacopo Mondi Sept. 25, 2020, 3:12 p.m. UTC | #1 
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 ?

> 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@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 ?

Reviewed-by: Jacopo Mondi <jacopo@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Niklas Söderlund Sept. 25, 2020, 4:53 p.m. UTC | #2 
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@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@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@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi Sept. 28, 2020, 8:40 a.m. UTC | #3 
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@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@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@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
Laurent Pinchart Sept. 28, 2020, 10:12 p.m. UTC | #4 
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@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@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__ */

Patch
diff mbox series 

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. */
+	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__ */