[libcamera-devel] [PATCH v3 16/22] libcamera: pipeline: rkisp1: Move path configuration to RkISP1Path

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Fri, Sep 25, 2020 at 03:42:01AM +0200, Niklas Söderlund wrote:
> Move the path configuration to RkISP1Path to increase code reuse and
> make the V4L2 subdevice resizer private to the path. There is no
> functional change.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 55 +++-----------------
>  src/libcamera/pipeline/rkisp1/rkisp1path.cpp | 55 +++++++++++++++++++-
>  src/libcamera/pipeline/rkisp1/rkisp1path.h   | 10 +++-
>  3 files changed, 68 insertions(+), 52 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index e738a7eb19264d79..2403eec4691b5ff6 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -810,60 +810,17 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  	data->mainPathActive_ = false;
>  	data->selfPathActive_ = false;
>  	for (const StreamConfiguration &cfg : *config) {
> -		V4L2SubdeviceFormat ispFormat = format;
> -		V4L2Subdevice *resizer;
> -		V4L2VideoDevice *video;
> -
>  		if (cfg.stream() == &data->mainPathStream_) {
> -			resizer = mainPath_.resizer_;
> -			video = mainPath_.video_;
> +			ret = mainPath_.configure(cfg, format);
> +			if (ret)
> +				return ret;
>  			data->mainPathActive_ = true;
>  		} else {
> -			resizer = selfPath_.resizer_;
> -			video = selfPath_.video_;
> +			ret = selfPath_.configure(cfg, format);
> +			if (ret)
> +				return ret;
>  			data->selfPathActive_ = true;
>  		}
> -
> -		ret = resizer->setFormat(0, &ispFormat);
> -		if (ret < 0)
> -			return ret;
> -
> -		const char *name = resizer == mainPath_.resizer_ ? "main" : "self";
> -
> -		LOG(RkISP1, Debug)
> -			<< "Configured " << name << " resizer input pad with "
> -			<< ispFormat.toString();
> -
> -		ispFormat.size = cfg.size;
> -
> -		LOG(RkISP1, Debug)
> -			<< "Configuring " << name << " resizer output pad with "
> -			<< ispFormat.toString();
> -
> -		ret = resizer->setFormat(1, &ispFormat);
> -		if (ret < 0)
> -			return ret;
> -
> -		LOG(RkISP1, Debug)
> -			<< "Configured " << name << " resizer output pad with "
> -			<< ispFormat.toString();
> -
> -		const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> -		V4L2DeviceFormat outputFormat = {};
> -		outputFormat.fourcc = video->toV4L2PixelFormat(cfg.pixelFormat);
> -		outputFormat.size = cfg.size;
> -		outputFormat.planesCount = info.numPlanes();
> -
> -		ret = video->setFormat(&outputFormat);
> -		if (ret)
> -			return ret;
> -
> -		if (outputFormat.size != cfg.size ||
> -		    outputFormat.fourcc != video->toV4L2PixelFormat(cfg.pixelFormat)) {
> -			LOG(RkISP1, Error)
> -				<< "Unable to configure capture in " << cfg.toString();
> -			return -EINVAL;
> -		}
>  	}
>  
>  	V4L2DeviceFormat paramFormat = {};
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1path.cpp
> index 51a75df86baaaa7b..758580934817ed6a 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1path.cpp
> @@ -7,14 +7,18 @@
>  
>  #include "rkisp1path.h"
>  
> +#include <libcamera/stream.h>
> +
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/v4l2_subdevice.h"
>  #include "libcamera/internal/v4l2_videodevice.h"
>  
>  namespace libcamera {
>  
> +LOG_DECLARE_CATEGORY(RkISP1)
> +
>  RkISP1Path::RkISP1Path(const char *name)
> -	: resizer_(nullptr), video_(nullptr), name_(name)
> +	: video_(nullptr), name_(name), resizer_(nullptr)
>  {
>  }
>  
> @@ -40,6 +44,55 @@ bool RkISP1Path::init(MediaDevice *media)
>  	return true;
>  }
>  
> +int RkISP1Path::configure(const StreamConfiguration &config,
> +			  const V4L2SubdeviceFormat &inputFormat)
> +{
> +	int ret;
> +
> +	V4L2SubdeviceFormat ispFormat = inputFormat;
> +
> +	ret = resizer_->setFormat(0, &ispFormat);
> +	if (ret < 0)
> +		return ret;
> +
> +	LOG(RkISP1, Debug)
> +		<< "Configured " << name_ << " resizer input pad with "
> +		<< ispFormat.toString();
> +
> +	ispFormat.size = config.size;
> +
> +	LOG(RkISP1, Debug)
> +		<< "Configuring " << name_ << " resizer output pad with "
> +		<< ispFormat.toString();
> +
> +	ret = resizer_->setFormat(1, &ispFormat);
> +	if (ret < 0)
> +		return ret;
> +
> +	LOG(RkISP1, Debug)
> +		<< "Configured " << name_ << " resizer output pad with "
> +		<< ispFormat.toString();
> +
> +	const PixelFormatInfo &info = PixelFormatInfo::info(config.pixelFormat);
> +	V4L2DeviceFormat outputFormat = {};
> +	outputFormat.fourcc = video_->toV4L2PixelFormat(config.pixelFormat);
> +	outputFormat.size = config.size;
> +	outputFormat.planesCount = info.numPlanes();
> +
> +	ret = video_->setFormat(&outputFormat);
> +	if (ret)
> +		return ret;
> +
> +	if (outputFormat.size != config.size ||
> +	    outputFormat.fourcc != video_->toV4L2PixelFormat(config.pixelFormat)) {
> +		LOG(RkISP1, Error)
> +			<< "Unable to configure capture in " << config.toString();
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  RkISP1MainPath::RkISP1MainPath()
>  	: RkISP1Path("main")
>  {
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1path.h b/src/libcamera/pipeline/rkisp1/rkisp1path.h
> index d3172e228a3f67bf..6eb01529d2fddb1c 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1path.h
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1path.h
> @@ -12,6 +12,8 @@ namespace libcamera {
>  class MediaDevice;
>  class V4L2Subdevice;
>  class V4L2VideoDevice;
> +struct StreamConfiguration;
> +struct V4L2SubdeviceFormat;
>  
>  class RkISP1Path
>  {
> @@ -21,12 +23,16 @@ public:
>  
>  	bool init(MediaDevice *media);
>  
> -	/* \todo Make resizer and video private. */
> -	V4L2Subdevice *resizer_;
> +	int configure(const StreamConfiguration &config,
> +		      const V4L2SubdeviceFormat &inputFormat);
> +
> +	/* \todo Make video private. */
>  	V4L2VideoDevice *video_;
>  
>  private:
>  	const char *name_;
> +
> +	V4L2Subdevice *resizer_;
>  };
>  
>  class RkISP1MainPath : public RkISP1Path

-- 
Regards,

Laurent Pinchart