[libcamera-devel] [PATCH v3 03/16] libcamera: ipu3: Rationalize constant expressions names
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Oct 14 11:02:23 CEST 2021
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 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;
I'm all in favour of using constexprs in this series so:
Reviewed-by: Kieran Bingham <kieran.bingham at 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 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
More information about the libcamera-devel
mailing list