[libcamera-devel] [PATCH v3 03/16] libcamera: ipu3: Rationalize constant expressions names

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Oct 11, 2021 at 05:11:41PM +0200, Jacopo Mondi wrote:
> Following the previous patch that moved all the ImgU-related contants in
> the ImgUDevice class namespace and that aligned their naming scheme to
> the 'kNameOfConstant' scheme, apply the same changes to the other
> components of the IPU3 pipeline handler.
> 
> Cosmetic change, no functional changes intended.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/cio2.cpp |  6 +++---
>  src/libcamera/pipeline/ipu3/cio2.h   |  2 +-
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 29 ++++++++++++++--------------
>  3 files changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index dc62ab197acb..59dda56bdd3d 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -234,7 +234,7 @@ StreamConfiguration CIO2Device::generateConfiguration(Size size) const
>  
>  	cfg.size = sensorFormat.size;
>  	cfg.pixelFormat = mbusCodesToPixelFormat.at(sensorFormat.mbus_code);
> -	cfg.bufferCount = CIO2_BUFFER_COUNT;
> +	cfg.bufferCount = kBufferCount;
>  
>  	return cfg;
>  }
> @@ -338,11 +338,11 @@ int CIO2Device::exportBuffers(unsigned int count,
>  
>  int CIO2Device::start()
>  {
> -	int ret = output_->exportBuffers(CIO2_BUFFER_COUNT, &buffers_);
> +	int ret = output_->exportBuffers(kBufferCount, &buffers_);
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = output_->importBuffers(CIO2_BUFFER_COUNT);
> +	ret = output_->importBuffers(kBufferCount);
>  	if (ret)
>  		LOG(IPU3, Error) << "Failed to import CIO2 buffers";
>  
> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> index 5fffe921f213..ba8f0052c5b3 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.h
> +++ b/src/libcamera/pipeline/ipu3/cio2.h
> @@ -30,7 +30,7 @@ struct StreamConfiguration;
>  class CIO2Device
>  {
>  public:
> -	static constexpr unsigned int CIO2_BUFFER_COUNT = 4;
> +	static constexpr unsigned int kBufferCount = 4;
>  
>  	CIO2Device();
>  
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index bb3826eee422..6449fa543191 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -39,10 +39,6 @@ namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(IPU3)
>  
> -static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> -static constexpr unsigned int IPU3_MAX_STREAMS = 3;
> -static constexpr Size IPU3ViewfinderSize(1280, 720);
> -
>  static const ControlInfoMap::Map IPU3Controls = {
>  	{ &controls::draft::PipelineDepth, ControlInfo(2, 3) },
>  };
> @@ -93,6 +89,9 @@ private:
>  class IPU3CameraConfiguration : public CameraConfiguration
>  {
>  public:
> +	static constexpr unsigned int kBufferCount = 4;
> +	static constexpr unsigned int kMaxStreams = 3;
> +
>  	IPU3CameraConfiguration(IPU3CameraData *data);
>  
>  	Status validate() override;
> @@ -118,7 +117,8 @@ private:
>  class PipelineHandlerIPU3 : public PipelineHandler
>  {
>  public:
> -	static constexpr unsigned int V4L2_CID_IPU3_PIPE_MODE = 0x009819c1;
> +	static constexpr unsigned int kPipeModeCtrl = 0x009819c1;

This I would keep as-is, as I think it should be moved to the
intel-ipu3.h kernel header.

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

> +	static constexpr Size kViewfinderSize{ 1280, 720 };
>  
>  	enum IPU3PipeModes {
>  		IPU3PipeModeVideo = 0,
> @@ -218,8 +218,8 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  	combinedTransform_ = combined;
>  
>  	/* Cap the number of entries to the available streams. */
> -	if (config_.size() > IPU3_MAX_STREAMS) {
> -		config_.resize(IPU3_MAX_STREAMS);
> +	if (config_.size() > kMaxStreams) {
> +		config_.resize(kMaxStreams);
>  		status = Adjusted;
>  	}
>  
> @@ -355,7 +355,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  					      ImgUDevice::kOutputAlignHeight);
>  
>  			cfg->pixelFormat = formats::NV12;
> -			cfg->bufferCount = IPU3_BUFFER_COUNT;
> +			cfg->bufferCount = kBufferCount;
>  			cfg->stride = info.stride(cfg->size.width, 0, 1);
>  			cfg->frameSize = info.frameSize(cfg->size, 1);
>  
> @@ -444,7 +444,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>  			size.height = utils::alignDown(size.height - 1,
>  						       ImgUDevice::kOutputMarginHeight);
>  			pixelFormat = formats::NV12;
> -			bufferCount = IPU3_BUFFER_COUNT;
> +			bufferCount = IPU3CameraConfiguration::kBufferCount;
>  			streamFormats[pixelFormat] = { { ImgUDevice::kOutputMinSize, size } };
>  
>  			break;
> @@ -469,11 +469,11 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>  			 * capped to the maximum sensor resolution and aligned
>  			 * to the ImgU output constraints.
>  			 */
> -			size = sensorResolution.boundedTo(IPU3ViewfinderSize)
> +			size = sensorResolution.boundedTo(kViewfinderSize)
>  					       .alignedDownTo(ImgUDevice::kOutputAlignWidth,
>  							      ImgUDevice::kOutputAlignHeight);
>  			pixelFormat = formats::NV12;
> -			bufferCount = IPU3_BUFFER_COUNT;
> +			bufferCount = IPU3CameraConfiguration::kBufferCount;
>  			streamFormats[pixelFormat] = { { ImgUDevice::kOutputMinSize, size } };
>  
>  			break;
> @@ -645,8 +645,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  	 * \todo Figure out what the 'Still Capture' mode is meant for, and use
>  	 * it accordingly.
>  	 */
> -	ctrls.set(V4L2_CID_IPU3_PIPE_MODE,
> -		  static_cast<int32_t>(IPU3PipeModeVideo));
> +	ctrls.set(kPipeModeCtrl, static_cast<int32_t>(IPU3PipeModeVideo));
>  	ret = imgu->imgu_->setControls(&ctrls);
>  	if (ret) {
>  		LOG(IPU3, Error) << "Unable to set pipe_mode control";
> @@ -1005,8 +1004,8 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>  	 * Either the smallest margin-aligned size larger than the viewfinder
>  	 * size or the adjusted sensor resolution.
>  	 */
> -	minSize = Size(IPU3ViewfinderSize.width + 1,
> -		       IPU3ViewfinderSize.height + 1)
> +	minSize = Size(kViewfinderSize.width + 1,
> +		       kViewfinderSize.height + 1)
>  		  .alignedUpTo(ImgUDevice::kOutputMarginWidth,
>  			       ImgUDevice::kOutputMarginHeight)
>  		  .boundedTo(minSize);

-- 
Regards,

Laurent Pinchart