[libcamera-devel] [PATCH v3 03/16] libcamera: ipu3: Rationalize constant expressions names
Jacopo Mondi
jacopo at jmondi.org
Thu Oct 14 14:41:51 CEST 2021
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 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.
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 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