[{"id":20158,"web_url":"https://patchwork.libcamera.org/comment/20158/","msgid":"","date":"2021-10-13T01:20:08","subject":"Re: [libcamera-devel] [PATCH v3 03/16] libcamera: ipu3: Rationalize\n\tconstant expressions names","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Mon, Oct 11, 2021 at 05:11:41PM +0200, Jacopo Mondi wrote:\n> Following the previous patch that moved all the ImgU-related contants in\n> the ImgUDevice class namespace and that aligned their naming scheme to\n> the 'kNameOfConstant' scheme, apply the same changes to the other\n> components of the IPU3 pipeline handler.\n> \n> Cosmetic change, no functional changes intended.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/cio2.cpp | 6 +++---\n> src/libcamera/pipeline/ipu3/cio2.h | 2 +-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 29 ++++++++++++++--------------\n> 3 files changed, 18 insertions(+), 19 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> index dc62ab197acb..59dda56bdd3d 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> @@ -234,7 +234,7 @@ StreamConfiguration CIO2Device::generateConfiguration(Size size) const\n> \n> \tcfg.size = sensorFormat.size;\n> \tcfg.pixelFormat = mbusCodesToPixelFormat.at(sensorFormat.mbus_code);\n> -\tcfg.bufferCount = CIO2_BUFFER_COUNT;\n> +\tcfg.bufferCount = kBufferCount;\n> \n> \treturn cfg;\n> }\n> @@ -338,11 +338,11 @@ int CIO2Device::exportBuffers(unsigned int count,\n> \n> int CIO2Device::start()\n> {\n> -\tint ret = output_->exportBuffers(CIO2_BUFFER_COUNT, &buffers_);\n> +\tint ret = output_->exportBuffers(kBufferCount, &buffers_);\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> -\tret = output_->importBuffers(CIO2_BUFFER_COUNT);\n> +\tret = output_->importBuffers(kBufferCount);\n> \tif (ret)\n> \t\tLOG(IPU3, Error) << \"Failed to import CIO2 buffers\";\n> \n> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h\n> index 5fffe921f213..ba8f0052c5b3 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.h\n> +++ b/src/libcamera/pipeline/ipu3/cio2.h\n> @@ -30,7 +30,7 @@ struct StreamConfiguration;\n> class CIO2Device\n> {\n> public:\n> -\tstatic constexpr unsigned int CIO2_BUFFER_COUNT = 4;\n> +\tstatic constexpr unsigned int kBufferCount = 4;\n> \n> \tCIO2Device();\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index bb3826eee422..6449fa543191 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -39,10 +39,6 @@ namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(IPU3)\n> \n> -static constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> -static constexpr unsigned int IPU3_MAX_STREAMS = 3;\n> -static constexpr Size IPU3ViewfinderSize(1280, 720);\n> -\n> static const ControlInfoMap::Map IPU3Controls = {\n> \t{ &controls::draft::PipelineDepth, ControlInfo(2, 3) },\n> };\n> @@ -93,6 +89,9 @@ private:\n> class IPU3CameraConfiguration : public CameraConfiguration\n> {\n> public:\n> +\tstatic constexpr unsigned int kBufferCount = 4;\n> +\tstatic constexpr unsigned int kMaxStreams = 3;\n> +\n> \tIPU3CameraConfiguration(IPU3CameraData *data);\n> \n> \tStatus validate() override;\n> @@ -118,7 +117,8 @@ private:\n> class PipelineHandlerIPU3 : public PipelineHandler\n> {\n> public:\n> -\tstatic constexpr unsigned int V4L2_CID_IPU3_PIPE_MODE = 0x009819c1;\n> +\tstatic constexpr unsigned int kPipeModeCtrl = 0x009819c1;\n\nThis I would keep as-is, as I think it should be moved to the\nintel-ipu3.h kernel header.\n\nReviewed-by: Laurent Pinchart \n\n> +\tstatic constexpr Size kViewfinderSize{ 1280, 720 };\n> \n> \tenum IPU3PipeModes {\n> \t\tIPU3PipeModeVideo = 0,\n> @@ -218,8 +218,8 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \tcombinedTransform_ = combined;\n> \n> \t/* Cap the number of entries to the available streams. */\n> -\tif (config_.size() > IPU3_MAX_STREAMS) {\n> -\t\tconfig_.resize(IPU3_MAX_STREAMS);\n> +\tif (config_.size() > kMaxStreams) {\n> +\t\tconfig_.resize(kMaxStreams);\n> \t\tstatus = Adjusted;\n> \t}\n> \n> @@ -355,7 +355,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \t\t\t\t\t ImgUDevice::kOutputAlignHeight);\n> \n> \t\t\tcfg->pixelFormat = formats::NV12;\n> -\t\t\tcfg->bufferCount = IPU3_BUFFER_COUNT;\n> +\t\t\tcfg->bufferCount = kBufferCount;\n> \t\t\tcfg->stride = info.stride(cfg->size.width, 0, 1);\n> \t\t\tcfg->frameSize = info.frameSize(cfg->size, 1);\n> \n> @@ -444,7 +444,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> \t\t\tsize.height = utils::alignDown(size.height - 1,\n> \t\t\t\t\t\t ImgUDevice::kOutputMarginHeight);\n> \t\t\tpixelFormat = formats::NV12;\n> -\t\t\tbufferCount = IPU3_BUFFER_COUNT;\n> +\t\t\tbufferCount = IPU3CameraConfiguration::kBufferCount;\n> \t\t\tstreamFormats[pixelFormat] = { { ImgUDevice::kOutputMinSize, size } };\n> \n> \t\t\tbreak;\n> @@ -469,11 +469,11 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> \t\t\t * capped to the maximum sensor resolution and aligned\n> \t\t\t * to the ImgU output constraints.\n> \t\t\t */\n> -\t\t\tsize = sensorResolution.boundedTo(IPU3ViewfinderSize)\n> +\t\t\tsize = sensorResolution.boundedTo(kViewfinderSize)\n> \t\t\t\t\t .alignedDownTo(ImgUDevice::kOutputAlignWidth,\n> \t\t\t\t\t\t\t ImgUDevice::kOutputAlignHeight);\n> \t\t\tpixelFormat = formats::NV12;\n> -\t\t\tbufferCount = IPU3_BUFFER_COUNT;\n> +\t\t\tbufferCount = IPU3CameraConfiguration::kBufferCount;\n> \t\t\tstreamFormats[pixelFormat] = { { ImgUDevice::kOutputMinSize, size } };\n> \n> \t\t\tbreak;\n> @@ -645,8 +645,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> \t * \\todo Figure out what the 'Still Capture' mode is meant for, and use\n> \t * it accordingly.\n> \t */\n> -\tctrls.set(V4L2_CID_IPU3_PIPE_MODE,\n> -\t\t static_cast(IPU3PipeModeVideo));\n> +\tctrls.set(kPipeModeCtrl, static_cast(IPU3PipeModeVideo));\n> \tret = imgu->imgu_->setControls(&ctrls);\n> \tif (ret) {\n> \t\tLOG(IPU3, Error) << \"Unable to set pipe_mode control\";\n> @@ -1005,8 +1004,8 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> \t * Either the smallest margin-aligned size larger than the viewfinder\n> \t * size or the adjusted sensor resolution.\n> \t */\n> -\tminSize = Size(IPU3ViewfinderSize.width + 1,\n> -\t\t IPU3ViewfinderSize.height + 1)\n> +\tminSize = Size(kViewfinderSize.width + 1,\n> +\t\t kViewfinderSize.height + 1)\n> \t\t .alignedUpTo(ImgUDevice::kOutputMarginWidth,\n> \t\t\t ImgUDevice::kOutputMarginHeight)\n> \t\t .boundedTo(minSize);","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CBD40C323E\n\tfor ;\n\tWed, 13 Oct 2021 01:20:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4051968F4F;\n\tWed, 13 Oct 2021 03:20:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F35446012B\n\tfor ;\n\tWed, 13 Oct 2021 03:20:22 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 753B4291;\n\tWed, 13 Oct 2021 03:20:22 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"WhtzKZOk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634088022;\n\tbh=AA84j+anIgUUUzgUl5Dh99mGBBt3tAKEeLkVKNzJZEU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WhtzKZOkJROT4b6Etc5ktmnw4ezHC0E8zLW+1NhGeXJVXDLUUInhJfDEfwnxeYh8x\n\t+NeD6XDTJ9WtRluE+tdXc052glikl1lKQjD2Nr4vCboeePg7DPjiS1rQrJ/3tNeVlh\n\tmrdl/gRlYbV9PXTQfh0r20qBYysNVA83HLHpZiCk=","Date":"Wed, 13 Oct 2021 04:20:08 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211011151154.72856-1-jacopo@jmondi.org>\n\t<20211011151154.72856-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211011151154.72856-4-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 03/16] libcamera: ipu3: Rationalize\n\tconstant expressions names","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20188,"web_url":"https://patchwork.libcamera.org/comment/20188/","msgid":"<163420214331.3654017.13048677199103524313@Monstersaurus>","date":"2021-10-14T09:02:23","subject":"Re: [libcamera-devel] [PATCH v3 03/16] libcamera: ipu3: Rationalize\n\tconstant expressions names","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2021-10-13 02:20:08)\n> Hi Jacopo,\n> \n> Thank you for the patch.\n> \n> On Mon, Oct 11, 2021 at 05:11:41PM +0200, Jacopo Mondi wrote:\n> > Following the previous patch that moved all the ImgU-related contants in\n> > the ImgUDevice class namespace and that aligned their naming scheme to\n> > the 'kNameOfConstant' scheme, apply the same changes to the other\n> > components of the IPU3 pipeline handler.\n> > \n> > Cosmetic change, no functional changes intended.\n> > \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/cio2.cpp | 6 +++---\n> > src/libcamera/pipeline/ipu3/cio2.h | 2 +-\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 29 ++++++++++++++--------------\n> > 3 files changed, 18 insertions(+), 19 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > index dc62ab197acb..59dda56bdd3d 100644\n> > --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > @@ -234,7 +234,7 @@ StreamConfiguration CIO2Device::generateConfiguration(Size size) const\n> > \n> > cfg.size = sensorFormat.size;\n> > cfg.pixelFormat = mbusCodesToPixelFormat.at(sensorFormat.mbus_code);\n> > - cfg.bufferCount = CIO2_BUFFER_COUNT;\n> > + cfg.bufferCount = kBufferCount;\n> > \n> > return cfg;\n> > }\n> > @@ -338,11 +338,11 @@ int CIO2Device::exportBuffers(unsigned int count,\n> > \n> > int CIO2Device::start()\n> > {\n> > - int ret = output_->exportBuffers(CIO2_BUFFER_COUNT, &buffers_);\n> > + int ret = output_->exportBuffers(kBufferCount, &buffers_);\n> > if (ret < 0)\n> > return ret;\n> > \n> > - ret = output_->importBuffers(CIO2_BUFFER_COUNT);\n> > + ret = output_->importBuffers(kBufferCount);\n> > if (ret)\n> > LOG(IPU3, Error) << \"Failed to import CIO2 buffers\";\n> > \n> > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h\n> > index 5fffe921f213..ba8f0052c5b3 100644\n> > --- a/src/libcamera/pipeline/ipu3/cio2.h\n> > +++ b/src/libcamera/pipeline/ipu3/cio2.h\n> > @@ -30,7 +30,7 @@ struct StreamConfiguration;\n> > class CIO2Device\n> > {\n> > public:\n> > - static constexpr unsigned int CIO2_BUFFER_COUNT = 4;\n> > + static constexpr unsigned int kBufferCount = 4;\n> > \n> > CIO2Device();\n> > \n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index bb3826eee422..6449fa543191 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -39,10 +39,6 @@ namespace libcamera {\n> > \n> > LOG_DEFINE_CATEGORY(IPU3)\n> > \n> > -static constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> > -static constexpr unsigned int IPU3_MAX_STREAMS = 3;\n> > -static constexpr Size IPU3ViewfinderSize(1280, 720);\n> > -\n> > static const ControlInfoMap::Map IPU3Controls = {\n> > { &controls::draft::PipelineDepth, ControlInfo(2, 3) },\n> > };\n> > @@ -93,6 +89,9 @@ private:\n> > class IPU3CameraConfiguration : public CameraConfiguration\n> > {\n> > public:\n> > + static constexpr unsigned int kBufferCount = 4;\n\nI'm all in favour of using constexprs in this series so:\n\nReviewed-by: Kieran Bingham \n\nOnly an unrelated question. Will kBufferCount always be constant? I\nguess it can be as there shouldn't be need for more than 4 buffers\nbetween the CIO2 and the IMGU.\n\n\n> > + static constexpr unsigned int kMaxStreams = 3;\n> > +\n> > IPU3CameraConfiguration(IPU3CameraData *data);\n> > \n> > Status validate() override;\n> > @@ -118,7 +117,8 @@ private:\n> > class PipelineHandlerIPU3 : public PipelineHandler\n> > {\n> > public:\n> > - static constexpr unsigned int V4L2_CID_IPU3_PIPE_MODE = 0x009819c1;\n> > + static constexpr unsigned int kPipeModeCtrl = 0x009819c1;\n> \n> This I would keep as-is, as I think it should be moved to the\n> intel-ipu3.h kernel header.\n> \n> Reviewed-by: Laurent Pinchart \n> \n> > + static constexpr Size kViewfinderSize{ 1280, 720 };\n> > \n> > enum IPU3PipeModes {\n> > IPU3PipeModeVideo = 0,\n> > @@ -218,8 +218,8 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > combinedTransform_ = combined;\n> > \n> > /* Cap the number of entries to the available streams. */\n> > - if (config_.size() > IPU3_MAX_STREAMS) {\n> > - config_.resize(IPU3_MAX_STREAMS);\n> > + if (config_.size() > kMaxStreams) {\n> > + config_.resize(kMaxStreams);\n> > status = Adjusted;\n> > }\n> > \n> > @@ -355,7 +355,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > ImgUDevice::kOutputAlignHeight);\n> > \n> > cfg->pixelFormat = formats::NV12;\n> > - cfg->bufferCount = IPU3_BUFFER_COUNT;\n> > + cfg->bufferCount = kBufferCount;\n> > cfg->stride = info.stride(cfg->size.width, 0, 1);\n> > cfg->frameSize = info.frameSize(cfg->size, 1);\n> > \n> > @@ -444,7 +444,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > size.height = utils::alignDown(size.height - 1,\n> > ImgUDevice::kOutputMarginHeight);\n> > pixelFormat = formats::NV12;\n> > - bufferCount = IPU3_BUFFER_COUNT;\n> > + bufferCount = IPU3CameraConfiguration::kBufferCount;\n> > streamFormats[pixelFormat] = { { ImgUDevice::kOutputMinSize, size } };\n> > \n> > break;\n> > @@ -469,11 +469,11 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > * capped to the maximum sensor resolution and aligned\n> > * to the ImgU output constraints.\n> > */\n> > - size = sensorResolution.boundedTo(IPU3ViewfinderSize)\n> > + size = sensorResolution.boundedTo(kViewfinderSize)\n> > .alignedDownTo(ImgUDevice::kOutputAlignWidth,\n> > ImgUDevice::kOutputAlignHeight);\n> > pixelFormat = formats::NV12;\n> > - bufferCount = IPU3_BUFFER_COUNT;\n> > + bufferCount = IPU3CameraConfiguration::kBufferCount;\n> > streamFormats[pixelFormat] = { { ImgUDevice::kOutputMinSize, size } };\n> > \n> > break;\n> > @@ -645,8 +645,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > * \\todo Figure out what the 'Still Capture' mode is meant for, and use\n> > * it accordingly.\n> > */\n> > - ctrls.set(V4L2_CID_IPU3_PIPE_MODE,\n> > - static_cast(IPU3PipeModeVideo));\n> > + ctrls.set(kPipeModeCtrl, static_cast(IPU3PipeModeVideo));\n> > ret = imgu->imgu_->setControls(&ctrls);\n> > if (ret) {\n> > LOG(IPU3, Error) << \"Unable to set pipe_mode control\";\n> > @@ -1005,8 +1004,8 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > * Either the smallest margin-aligned size larger than the viewfinder\n> > * size or the adjusted sensor resolution.\n> > */\n> > - minSize = Size(IPU3ViewfinderSize.width + 1,\n> > - IPU3ViewfinderSize.height + 1)\n> > + minSize = Size(kViewfinderSize.width + 1,\n> > + kViewfinderSize.height + 1)\n> > .alignedUpTo(ImgUDevice::kOutputMarginWidth,\n> > ImgUDevice::kOutputMarginHeight)\n> > .boundedTo(minSize);\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id F3B2AC323E\n\tfor ;\n\tThu, 14 Oct 2021 09:02:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 79FBF68F52;\n\tThu, 14 Oct 2021 11:02:27 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6380968F4A\n\tfor ;\n\tThu, 14 Oct 2021 11:02:26 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E42682F3;\n\tThu, 14 Oct 2021 11:02:25 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"g/0d/Jee\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634202146;\n\tbh=mt0w0VFruRwsD8Pmt60rQPSHB1VOd8J+iRrxpHELrVY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=g/0d/Jeezu4V/2583bAg0Vh6AyVk/xpvir15YADrtCjQCVhnOJOX/DVLzvjaNi60T\n\t27jZhUvrjekMXip9JgHL2uc2d+erlbQ55dY0RZtz35cmxnTiE6o9jUK+3zsZQsGl+E\n\tYNmJk5NGWYgw/o309/989wRjNxGPSpQAKsBEXR1U=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211011151154.72856-1-jacopo@jmondi.org>\n\t<20211011151154.72856-4-jacopo@jmondi.org>\n\t","From":"Kieran Bingham ","To":"Jacopo Mondi ,\n\tLaurent Pinchart ","Date":"Thu, 14 Oct 2021 10:02:23 +0100","Message-ID":"<163420214331.3654017.13048677199103524313@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v3 03/16] libcamera: ipu3: Rationalize\n\tconstant expressions names","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20207,"web_url":"https://patchwork.libcamera.org/comment/20207/","msgid":"<20211014124151.agzieyo2alyh2snh@uno.localdomain>","date":"2021-10-14T12:41:51","subject":"Re: [libcamera-devel] [PATCH v3 03/16] libcamera: ipu3: Rationalize\n\tconstant expressions names","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Thu, Oct 14, 2021 at 10:02:23AM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2021-10-13 02:20:08)\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n> >\n> > On Mon, Oct 11, 2021 at 05:11:41PM +0200, Jacopo Mondi wrote:\n> > > Following the previous patch that moved all the ImgU-related contants in\n> > > the ImgUDevice class namespace and that aligned their naming scheme to\n> > > the 'kNameOfConstant' scheme, apply the same changes to the other\n> > > components of the IPU3 pipeline handler.\n> > >\n> > > Cosmetic change, no functional changes intended.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/pipeline/ipu3/cio2.cpp | 6 +++---\n> > > src/libcamera/pipeline/ipu3/cio2.h | 2 +-\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 29 ++++++++++++++--------------\n> > > 3 files changed, 18 insertions(+), 19 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > index dc62ab197acb..59dda56bdd3d 100644\n> > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > @@ -234,7 +234,7 @@ StreamConfiguration CIO2Device::generateConfiguration(Size size) const\n> > >\n> > > cfg.size = sensorFormat.size;\n> > > cfg.pixelFormat = mbusCodesToPixelFormat.at(sensorFormat.mbus_code);\n> > > - cfg.bufferCount = CIO2_BUFFER_COUNT;\n> > > + cfg.bufferCount = kBufferCount;\n> > >\n> > > return cfg;\n> > > }\n> > > @@ -338,11 +338,11 @@ int CIO2Device::exportBuffers(unsigned int count,\n> > >\n> > > int CIO2Device::start()\n> > > {\n> > > - int ret = output_->exportBuffers(CIO2_BUFFER_COUNT, &buffers_);\n> > > + int ret = output_->exportBuffers(kBufferCount, &buffers_);\n> > > if (ret < 0)\n> > > return ret;\n> > >\n> > > - ret = output_->importBuffers(CIO2_BUFFER_COUNT);\n> > > + ret = output_->importBuffers(kBufferCount);\n> > > if (ret)\n> > > LOG(IPU3, Error) << \"Failed to import CIO2 buffers\";\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h\n> > > index 5fffe921f213..ba8f0052c5b3 100644\n> > > --- a/src/libcamera/pipeline/ipu3/cio2.h\n> > > +++ b/src/libcamera/pipeline/ipu3/cio2.h\n> > > @@ -30,7 +30,7 @@ struct StreamConfiguration;\n> > > class CIO2Device\n> > > {\n> > > public:\n> > > - static constexpr unsigned int CIO2_BUFFER_COUNT = 4;\n> > > + static constexpr unsigned int kBufferCount = 4;\n> > >\n> > > CIO2Device();\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index bb3826eee422..6449fa543191 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -39,10 +39,6 @@ namespace libcamera {\n> > >\n> > > LOG_DEFINE_CATEGORY(IPU3)\n> > >\n> > > -static constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> > > -static constexpr unsigned int IPU3_MAX_STREAMS = 3;\n> > > -static constexpr Size IPU3ViewfinderSize(1280, 720);\n> > > -\n> > > static const ControlInfoMap::Map IPU3Controls = {\n> > > { &controls::draft::PipelineDepth, ControlInfo(2, 3) },\n> > > };\n> > > @@ -93,6 +89,9 @@ private:\n> > > class IPU3CameraConfiguration : public CameraConfiguration\n> > > {\n> > > public:\n> > > + static constexpr unsigned int kBufferCount = 4;\n>\n> I'm all in favour of using constexprs in this series so:\n>\n> Reviewed-by: Kieran Bingham \n>\n> Only an unrelated question. Will kBufferCount always be constant? I\n> guess it can be as there shouldn't be need for more than 4 buffers\n> between the CIO2 and the IMGU.\n\nSo far a constant number of shared buffers between the CIO2 and the\nImgU have been enough. I guess this concur in defining our pipeline\ndepth, as the more buffers we have to keep the CIO2->ImgU machinery\ngoing the more we can have requests in-flight ?\n\n>\n>\n> > > + static constexpr unsigned int kMaxStreams = 3;\n> > > +\n> > > IPU3CameraConfiguration(IPU3CameraData *data);\n> > >\n> > > Status validate() override;\n> > > @@ -118,7 +117,8 @@ private:\n> > > class PipelineHandlerIPU3 : public PipelineHandler\n> > > {\n> > > public:\n> > > - static constexpr unsigned int V4L2_CID_IPU3_PIPE_MODE = 0x009819c1;\n> > > + static constexpr unsigned int kPipeModeCtrl = 0x009819c1;\n> >\n> > This I would keep as-is, as I think it should be moved to the\n> > intel-ipu3.h kernel header.\n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> > > + static constexpr Size kViewfinderSize{ 1280, 720 };\n> > >\n> > > enum IPU3PipeModes {\n> > > IPU3PipeModeVideo = 0,\n> > > @@ -218,8 +218,8 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > combinedTransform_ = combined;\n> > >\n> > > /* Cap the number of entries to the available streams. */\n> > > - if (config_.size() > IPU3_MAX_STREAMS) {\n> > > - config_.resize(IPU3_MAX_STREAMS);\n> > > + if (config_.size() > kMaxStreams) {\n> > > + config_.resize(kMaxStreams);\n> > > status = Adjusted;\n> > > }\n> > >\n> > > @@ -355,7 +355,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > ImgUDevice::kOutputAlignHeight);\n> > >\n> > > cfg->pixelFormat = formats::NV12;\n> > > - cfg->bufferCount = IPU3_BUFFER_COUNT;\n> > > + cfg->bufferCount = kBufferCount;\n> > > cfg->stride = info.stride(cfg->size.width, 0, 1);\n> > > cfg->frameSize = info.frameSize(cfg->size, 1);\n> > >\n> > > @@ -444,7 +444,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > > size.height = utils::alignDown(size.height - 1,\n> > > ImgUDevice::kOutputMarginHeight);\n> > > pixelFormat = formats::NV12;\n> > > - bufferCount = IPU3_BUFFER_COUNT;\n> > > + bufferCount = IPU3CameraConfiguration::kBufferCount;\n> > > streamFormats[pixelFormat] = { { ImgUDevice::kOutputMinSize, size } };\n> > >\n> > > break;\n> > > @@ -469,11 +469,11 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > > * capped to the maximum sensor resolution and aligned\n> > > * to the ImgU output constraints.\n> > > */\n> > > - size = sensorResolution.boundedTo(IPU3ViewfinderSize)\n> > > + size = sensorResolution.boundedTo(kViewfinderSize)\n> > > .alignedDownTo(ImgUDevice::kOutputAlignWidth,\n> > > ImgUDevice::kOutputAlignHeight);\n> > > pixelFormat = formats::NV12;\n> > > - bufferCount = IPU3_BUFFER_COUNT;\n> > > + bufferCount = IPU3CameraConfiguration::kBufferCount;\n> > > streamFormats[pixelFormat] = { { ImgUDevice::kOutputMinSize, size } };\n> > >\n> > > break;\n> > > @@ -645,8 +645,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > > * \\todo Figure out what the 'Still Capture' mode is meant for, and use\n> > > * it accordingly.\n> > > */\n> > > - ctrls.set(V4L2_CID_IPU3_PIPE_MODE,\n> > > - static_cast(IPU3PipeModeVideo));\n> > > + ctrls.set(kPipeModeCtrl, static_cast(IPU3PipeModeVideo));\n> > > ret = imgu->imgu_->setControls(&ctrls);\n> > > if (ret) {\n> > > LOG(IPU3, Error) << \"Unable to set pipe_mode control\";\n> > > @@ -1005,8 +1004,8 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > > * Either the smallest margin-aligned size larger than the viewfinder\n> > > * size or the adjusted sensor resolution.\n> > > */\n> > > - minSize = Size(IPU3ViewfinderSize.width + 1,\n> > > - IPU3ViewfinderSize.height + 1)\n> > > + minSize = Size(kViewfinderSize.width + 1,\n> > > + kViewfinderSize.height + 1)\n> > > .alignedUpTo(ImgUDevice::kOutputMarginWidth,\n> > > ImgUDevice::kOutputMarginHeight)\n> > > .boundedTo(minSize);\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9682BC323E\n\tfor ;\n\tThu, 14 Oct 2021 12:41:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D9BC968F4F;\n\tThu, 14 Oct 2021 14:41:04 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6BF4868541\n\tfor ;\n\tThu, 14 Oct 2021 14:41:03 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 7435E40013;\n\tThu, 14 Oct 2021 12:41:02 +0000 (UTC)"],"Date":"Thu, 14 Oct 2021 14:41:51 +0200","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20211014124151.agzieyo2alyh2snh@uno.localdomain>","References":"<20211011151154.72856-1-jacopo@jmondi.org>\n\t<20211011151154.72856-4-jacopo@jmondi.org>\n\t\n\t<163420214331.3654017.13048677199103524313@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<163420214331.3654017.13048677199103524313@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v3 03/16] libcamera: ipu3: Rationalize\n\tconstant expressions names","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]