[libcamera-devel,v3,03/16] libcamera: ipu3: Rationalize constant expressions names
diff mbox series

Message ID 20211011151154.72856-4-jacopo@jmondi.org
State Superseded
Headers show
Series
  • IPU3 control info update and HAL frame durations
Related show

Commit Message

Jacopo Mondi Oct. 11, 2021, 3:11 p.m. UTC 
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@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(-)

Comments

Laurent Pinchart Oct. 13, 2021, 1:20 a.m. UTC | #1 
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@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@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);
Kieran Bingham Oct. 14, 2021, 9:02 a.m. UTC | #2 
Quoting Laurent Pinchart (2021-10-13 02:20:08)
> 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@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;

I'm all in favour of using constexprs in this series so:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Only an unrelated question. Will kBufferCount always be constant? I
guess it can be as there shouldn't be need for more than 4 buffers
between the CIO2 and the IMGU.


> > +     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@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
Jacopo Mondi Oct. 14, 2021, 12:41 p.m. UTC | #3 
Hi Kieran,

On Thu, Oct 14, 2021 at 10:02:23AM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2021-10-13 02:20:08)
> > 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@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;
>
> I'm all in favour of using constexprs in this series so:
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> Only an unrelated question. Will kBufferCount always be constant? I
> guess it can be as there shouldn't be need for more than 4 buffers
> between the CIO2 and the IMGU.

So far a constant number of shared buffers between the CIO2 and the
ImgU have been enough. I guess this concur in defining our pipeline
depth, as the more buffers we have to keep the CIO2->ImgU machinery
going the more we can have requests in-flight ?

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

Patch
diff mbox series 

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;
+	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);