[{"id":11279,"web_url":"https://patchwork.libcamera.org/comment/11279/","msgid":"<20200709125942.GG3875643@oden.dyn.berto.se>","date":"2020-07-09T12:59:42","subject":"Re: [libcamera-devel] [PATCH v2 02/20] libcamera: ipu3: Remove\n\tstreams from generateConfiguration","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2020-07-09 10:41:10 +0200, Jacopo Mondi wrote:\n> Remove stream assignment from the IPU3 pipeline handler\n> generateConfiguration() implementation.\n> \n> The function aims to provide a suitable default for the requested use\n> cases. Defer stream assignment to validation and only initialize sizes\n> and formats.\n> \n> Reviewed-by: Kieran Bingham \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 104 ++++++++-------------------\n> 1 file changed, 30 insertions(+), 74 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index af51fb2d1718..85d21b4db046 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -31,6 +31,14 @@ 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 unsigned int IPU3_OUTPUT_MAX_WIDTH = 4480;\n> +static constexpr unsigned int IPU3_OUTPUT_MAX_HEIGHT = 34004;\n\nJust checking is 34004 the max height?\n\n> +static const Size minIPU3OutputSize = { 2, 2 };\n> +static constexpr unsigned int IPU3_OUTPUT_WIDTH_ALIGN = 0x3f;\n> +static constexpr unsigned int IPU3_OUTPUT_HEIGHT_ALIGN = 0x3;\n> +\n> class IPU3CameraData : public CameraData\n> {\n> public:\n> @@ -61,9 +69,6 @@ public:\n> \tconst std::vector &streams() { return streams_; }\n> \n> private:\n> -\tstatic constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> -\tstatic constexpr unsigned int IPU3_MAX_STREAMS = 3;\n> -\n> \tvoid assignStreams();\n> \tvoid adjustStream(StreamConfiguration &cfg, bool scale);\n> \n> @@ -293,94 +298,51 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> {\n> \tIPU3CameraData *data = cameraData(camera);\n> \tIPU3CameraConfiguration *config = new IPU3CameraConfiguration(camera, data);\n> -\tstd::set streams = {\n> -\t\t&data->outStream_,\n> -\t\t&data->vfStream_,\n> -\t\t&data->rawStream_,\n> -\t};\n> \n> \tif (roles.empty())\n> \t\treturn config;\n> \n> +\tSize sensorResolution = data->cio2_.sensor()->resolution();\n> \tfor (const StreamRole role : roles) {\n> \t\tStreamConfiguration cfg = {};\n> -\t\tStream *stream = nullptr;\n> -\n> -\t\tcfg.pixelFormat = formats::NV12;\n> \n> \t\tswitch (role) {\n> \t\tcase StreamRole::StillCapture:\n> \t\t\t/*\n> -\t\t\t * Pick the output stream by default as the Viewfinder\n> -\t\t\t * and VideoRecording roles are not allowed on\n> -\t\t\t * the output stream.\n> -\t\t\t */\n> -\t\t\tif (streams.find(&data->outStream_) != streams.end()) {\n> -\t\t\t\tstream = &data->outStream_;\n> -\t\t\t} else if (streams.find(&data->vfStream_) != streams.end()) {\n> -\t\t\t\tstream = &data->vfStream_;\n> -\t\t\t} else {\n> -\t\t\t\tLOG(IPU3, Error)\n> -\t\t\t\t\t<< \"No stream available for requested role \"\n> -\t\t\t\t\t<< role;\n> -\t\t\t\tbreak;\n> -\t\t\t}\n> -\n> -\t\t\t/*\n> -\t\t\t * FIXME: Soraka: the maximum resolution reported by\n> -\t\t\t * both sensors (2592x1944 for ov5670 and 4224x3136 for\n> -\t\t\t * ov13858) are returned as default configurations but\n> -\t\t\t * they're not correctly processed by the ImgU.\n> -\t\t\t * Resolutions up tp 2560x1920 have been validated.\n> -\t\t\t *\n> -\t\t\t * \\todo Clarify ImgU alignment requirements.\n> +\t\t\t * Use the sensor resolution aligned to the ImgU\n> +\t\t\t * output constraints.\n> \t\t\t */\n> -\t\t\tcfg.size = { 2560, 1920 };\n> +\t\t\tcfg.size.width = std::min(sensorResolution.width,\n> +\t\t\t\t\t\t IPU3_OUTPUT_MAX_WIDTH);\n> +\t\t\tcfg.size.height = std::min(sensorResolution.height,\n> +\t\t\t\t\t\t IPU3_OUTPUT_MAX_HEIGHT);\n> +\t\t\tcfg.size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;\n> +\t\t\tcfg.size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;\n> +\t\t\tcfg.pixelFormat = formats::NV12;\n> +\t\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\n> \n> \t\t\tbreak;\n> \n> \t\tcase StreamRole::StillCaptureRaw: {\n> -\t\t\tif (streams.find(&data->rawStream_) == streams.end()) {\n> -\t\t\t\tLOG(IPU3, Error)\n> -\t\t\t\t\t<< \"Multiple raw streams are not supported\";\n> -\t\t\t\tbreak;\n> -\t\t\t}\n> -\n> -\t\t\tstream = &data->rawStream_;\n> -\n> -\t\t\tcfg.size = data->cio2_.sensor()->resolution();\n> +\t\t\tcfg = data->cio2_.generateConfiguration(sensorResolution);\n> +\t\t\tcfg.bufferCount = 1;\n\nSetting bufferCount here is goes against the idea of having \nCIO2Device::generateConfiguration() control all the parameters of the \nRAW stream. I know there was some discussion if returning a \nStreamConfiguration from the CIO2Device is a good idea or not. I still \nthink it's the best design but I'm open to change/discuss it. \n\nBut if we really want to modify parameters of it here I agree returning \na StreamConfiguration here is confusing, double so as bufferCount is set \nto CIO2_BUFFER_COUNT and then changed to 1 here.\n\n> \n> -\t\t\tcfg = data->cio2_.generateConfiguration(cfg.size);\n> \t\t\tbreak;\n> \t\t}\n> \n> \t\tcase StreamRole::Viewfinder:\n> \t\tcase StreamRole::VideoRecording: {\n> \t\t\t/*\n> -\t\t\t * We can't use the 'output' stream for viewfinder or\n> -\t\t\t * video capture roles.\n> -\t\t\t *\n> -\t\t\t * \\todo This is an artificial limitation until we\n> -\t\t\t * figure out the exact capabilities of the hardware.\n> +\t\t\t * Default viewfinder to 1280x720, capped to the maximum\n> +\t\t\t * sensor resolution and aligned to the ImgU output\n> +\t\t\t * constraints.\n\nThis is done for both Viewfinder and VideoRecording right?\n\n> \t\t\t */\n> -\t\t\tif (streams.find(&data->vfStream_) == streams.end()) {\n> -\t\t\t\tLOG(IPU3, Error)\n> -\t\t\t\t\t<< \"No stream available for requested role \"\n> -\t\t\t\t\t<< role;\n> -\t\t\t\tbreak;\n> -\t\t\t}\n> -\n> -\t\t\tstream = &data->vfStream_;\n> -\n> -\t\t\t/*\n> -\t\t\t * Align the default viewfinder size to the maximum\n> -\t\t\t * available sensor resolution and to the IPU3\n> -\t\t\t * alignment constraints.\n> -\t\t\t */\n> -\t\t\tconst Size &res = data->cio2_.sensor()->resolution();\n> -\t\t\tunsigned int width = std::min(1280U, res.width);\n> -\t\t\tunsigned int height = std::min(720U, res.height);\n> -\t\t\tcfg.size = { width & ~7, height & ~3 };\n> +\t\t\tcfg.size.width = std::min(1280U, sensorResolution.width);\n> +\t\t\tcfg.size.height = std::min(720U, sensorResolution.height);\n> +\t\t\tcfg.size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;\n> +\t\t\tcfg.size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;\n> +\t\t\tcfg.pixelFormat = formats::NV12;\n> +\t\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\n> \n> \t\t\tbreak;\n> \t\t}\n> @@ -388,16 +350,10 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> \t\tdefault:\n> \t\t\tLOG(IPU3, Error)\n> \t\t\t\t<< \"Requested stream role not supported: \" << role;\n> -\t\t\tbreak;\n\nI would keep the break as it is the style elsewhere in camera is to \nterminate the default case with either break, return or continue.\n\n> -\t\t}\n> -\n> -\t\tif (!stream) {\n> \t\t\tdelete config;\n> \t\t\treturn nullptr;\n> \t\t}\n> \n> -\t\tstreams.erase(stream);\n> -\n> \t\tconfig->addConfiguration(cfg);\n> \t}\n> \n> -- \n> 2.27.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 DC896BD792\n\tfor ;\n\tThu, 9 Jul 2020 12:59:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 65F7761020;\n\tThu, 9 Jul 2020 14:59:46 +0200 (CEST)","from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com\n\t[IPv6:2a00:1450:4864:20::22d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BFFB961020\n\tfor ;\n\tThu, 9 Jul 2020 14:59:44 +0200 (CEST)","by mail-lj1-x22d.google.com with SMTP id q4so2295449lji.2\n\tfor ;\n\tThu, 09 Jul 2020 05:59:44 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tr11sm784406ljc.66.2020.07.09.05.59.42\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 09 Jul 2020 05:59:43 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"XBxGWiOJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=O8+IOu/lXmOOUdJZ2zy8m8gZOKqB0XPMYvXGQel+q9s=;\n\tb=XBxGWiOJo8HXduUc2qNVdZhGV4hLzxf7hDbQrxRuZw9Bdy1WobDIfwD62/j28nNpPG\n\tRoM4XAn1BkTgjNUThft6KgUdnPHsdGNdhyOSgeuv/SgbozbnohRxbO/mlDA6usToiC7M\n\t0nAFbcUtFjkkzzctTwVUwP14tU9U0bAutxCxHFRj937zzMCIVceFBy5NuYRVVMJoLZkZ\n\tX8OL4OTVtq4Ywq+k5T12ijf4w/tWZZwy7epVOzcWEfogc1LgWYCRQPqLpaJRdO+VtDzI\n\tABJdPW/c9+ZI69/cPY3aWTDZeQ6VPXeuca1XgGRMAKomked9mTWoCLnqYfSze8PghH5G\n\tGITg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=O8+IOu/lXmOOUdJZ2zy8m8gZOKqB0XPMYvXGQel+q9s=;\n\tb=TTSioyWMZgGDZ9f9nM1vsBo9X5g+iRFsg+ZI8PqqNbjpo4lVZ+MO6Eo4ncox7BF/HC\n\tehHjjU1Gu9T1gElPDeu0btAZHJE6F2BErYwuheVMtUYwaYFXAsfgJErIUqtm8+wAgU+3\n\ttbFhF2Jg030hjhfXBMpWrKmXN6v7OFCVmxSZc6P7B2oQfRE8Ofz4FZjn1WshcSVj0uk7\n\tIYmhgQ0Fm8CfHmeC2wWEyZKmT6oGdHdY5WKp0Yu+QomWQeZutqfTZFJP3ZK0u0JATqq9\n\tuGqteMZKyXo0YZc+EB9pKlZEaglRtaCyAUkuanDEl1tFcnevgV+NkKyBq4QLjXigRXTr\n\tzgpw==","X-Gm-Message-State":"AOAM531Gg0IWnbviOt+yO+4S43MlVw6s/1wjaET3AtXg6foIiuPfT0AD\n\tKd98w1U7yMr5GzVPmGPXRxl2PwwhC1w=","X-Google-Smtp-Source":"ABdhPJzPsmQ6Ga2K1cNPaa+Gh0+dJbQHaYUR5RLyL87WHFx7TfcXncpYecxrMFXBdbFP9REYurLHZA==","X-Received":"by 2002:a05:651c:3c2:: with SMTP id\n\tf2mr37441532ljp.37.1594299583702; \n\tThu, 09 Jul 2020 05:59:43 -0700 (PDT)","Date":"Thu, 9 Jul 2020 14:59:42 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20200709125942.GG3875643@oden.dyn.berto.se>","References":"<20200709084128.5316-1-jacopo@jmondi.org>\n\t<20200709084128.5316-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200709084128.5316-3-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 02/20] libcamera: ipu3: Remove\n\tstreams from generateConfiguration","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","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11302,"web_url":"https://patchwork.libcamera.org/comment/11302/","msgid":"<20200710065854.3g5ntdslt3mjgxhe@uno.localdomain>","date":"2020-07-10T06:58:54","subject":"Re: [libcamera-devel] [PATCH v2 02/20] libcamera: ipu3: Remove\n\tstreams from generateConfiguration","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n thanks for review\n\nOn Thu, Jul 09, 2020 at 02:59:42PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2020-07-09 10:41:10 +0200, Jacopo Mondi wrote:\n> > Remove stream assignment from the IPU3 pipeline handler\n> > generateConfiguration() implementation.\n> >\n> > The function aims to provide a suitable default for the requested use\n> > cases. Defer stream assignment to validation and only initialize sizes\n> > and formats.\n> >\n> > Reviewed-by: Kieran Bingham \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 104 ++++++++-------------------\n> > 1 file changed, 30 insertions(+), 74 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index af51fb2d1718..85d21b4db046 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -31,6 +31,14 @@ 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 unsigned int IPU3_OUTPUT_MAX_WIDTH = 4480;\n> > +static constexpr unsigned int IPU3_OUTPUT_MAX_HEIGHT = 34004;\n>\n> Just checking is 34004 the max height?\n>\n\nIt seems so\ndrivers/staging/media/ipu3/ipu3.h-#define IPU3_OUTPUT_MAX_HEIGHT 34004U\n\n> > +static const Size minIPU3OutputSize = { 2, 2 };\n> > +static constexpr unsigned int IPU3_OUTPUT_WIDTH_ALIGN = 0x3f;\n> > +static constexpr unsigned int IPU3_OUTPUT_HEIGHT_ALIGN = 0x3;\n> > +\n> > class IPU3CameraData : public CameraData\n> > {\n> > public:\n> > @@ -61,9 +69,6 @@ public:\n> > \tconst std::vector &streams() { return streams_; }\n> >\n> > private:\n> > -\tstatic constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> > -\tstatic constexpr unsigned int IPU3_MAX_STREAMS = 3;\n> > -\n> > \tvoid assignStreams();\n> > \tvoid adjustStream(StreamConfiguration &cfg, bool scale);\n> >\n> > @@ -293,94 +298,51 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > {\n> > \tIPU3CameraData *data = cameraData(camera);\n> > \tIPU3CameraConfiguration *config = new IPU3CameraConfiguration(camera, data);\n> > -\tstd::set streams = {\n> > -\t\t&data->outStream_,\n> > -\t\t&data->vfStream_,\n> > -\t\t&data->rawStream_,\n> > -\t};\n> >\n> > \tif (roles.empty())\n> > \t\treturn config;\n> >\n> > +\tSize sensorResolution = data->cio2_.sensor()->resolution();\n> > \tfor (const StreamRole role : roles) {\n> > \t\tStreamConfiguration cfg = {};\n> > -\t\tStream *stream = nullptr;\n> > -\n> > -\t\tcfg.pixelFormat = formats::NV12;\n> >\n> > \t\tswitch (role) {\n> > \t\tcase StreamRole::StillCapture:\n> > \t\t\t/*\n> > -\t\t\t * Pick the output stream by default as the Viewfinder\n> > -\t\t\t * and VideoRecording roles are not allowed on\n> > -\t\t\t * the output stream.\n> > -\t\t\t */\n> > -\t\t\tif (streams.find(&data->outStream_) != streams.end()) {\n> > -\t\t\t\tstream = &data->outStream_;\n> > -\t\t\t} else if (streams.find(&data->vfStream_) != streams.end()) {\n> > -\t\t\t\tstream = &data->vfStream_;\n> > -\t\t\t} else {\n> > -\t\t\t\tLOG(IPU3, Error)\n> > -\t\t\t\t\t<< \"No stream available for requested role \"\n> > -\t\t\t\t\t<< role;\n> > -\t\t\t\tbreak;\n> > -\t\t\t}\n> > -\n> > -\t\t\t/*\n> > -\t\t\t * FIXME: Soraka: the maximum resolution reported by\n> > -\t\t\t * both sensors (2592x1944 for ov5670 and 4224x3136 for\n> > -\t\t\t * ov13858) are returned as default configurations but\n> > -\t\t\t * they're not correctly processed by the ImgU.\n> > -\t\t\t * Resolutions up tp 2560x1920 have been validated.\n> > -\t\t\t *\n> > -\t\t\t * \\todo Clarify ImgU alignment requirements.\n> > +\t\t\t * Use the sensor resolution aligned to the ImgU\n> > +\t\t\t * output constraints.\n> > \t\t\t */\n> > -\t\t\tcfg.size = { 2560, 1920 };\n> > +\t\t\tcfg.size.width = std::min(sensorResolution.width,\n> > +\t\t\t\t\t\t IPU3_OUTPUT_MAX_WIDTH);\n> > +\t\t\tcfg.size.height = std::min(sensorResolution.height,\n> > +\t\t\t\t\t\t IPU3_OUTPUT_MAX_HEIGHT);\n> > +\t\t\tcfg.size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;\n> > +\t\t\tcfg.size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;\n> > +\t\t\tcfg.pixelFormat = formats::NV12;\n> > +\t\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\n> >\n> > \t\t\tbreak;\n> >\n> > \t\tcase StreamRole::StillCaptureRaw: {\n> > -\t\t\tif (streams.find(&data->rawStream_) == streams.end()) {\n> > -\t\t\t\tLOG(IPU3, Error)\n> > -\t\t\t\t\t<< \"Multiple raw streams are not supported\";\n> > -\t\t\t\tbreak;\n> > -\t\t\t}\n> > -\n> > -\t\t\tstream = &data->rawStream_;\n> > -\n> > -\t\t\tcfg.size = data->cio2_.sensor()->resolution();\n> > +\t\t\tcfg = data->cio2_.generateConfiguration(sensorResolution);\n> > +\t\t\tcfg.bufferCount = 1;\n>\n> Setting bufferCount here is goes against the idea of having\n> CIO2Device::generateConfiguration() control all the parameters of the\n> RAW stream. I know there was some discussion if returning a\n> StreamConfiguration from the CIO2Device is a good idea or not. I still\n> think it's the best design but I'm open to change/discuss it.\n\nAh, this was not intentional in first place to be honest.\n\nI would be happy to discuss (later) what is the best to return from\nthe CIO2 configuration, as I would have preferred not returning a\nformat, but that's oke for now.\n\n>\n> But if we really want to modify parameters of it here I agree returning\n> a StreamConfiguration here is confusing, double so as bufferCount is set\n> to CIO2_BUFFER_COUNT and then changed to 1 here.\n>\n\nWhat confused me is the pattern\n\n\t\tcfg.size = data->cio2_.sensor()->resolution();\n\t\tcfg = data->cio2_.generateConfiguration(cfg.size);\n\nAs using cfg.size just a place holder to pass it to the\ngenerateConfiguration() function is not nice imho. But removing the\nassignement of the whole configuration was not intentional, so I'll\nfix it.\n\n\n> >\n> > -\t\t\tcfg = data->cio2_.generateConfiguration(cfg.size);\n> > \t\t\tbreak;\n> > \t\t}\n> >\n> > \t\tcase StreamRole::Viewfinder:\n> > \t\tcase StreamRole::VideoRecording: {\n> > \t\t\t/*\n> > -\t\t\t * We can't use the 'output' stream for viewfinder or\n> > -\t\t\t * video capture roles.\n> > -\t\t\t *\n> > -\t\t\t * \\todo This is an artificial limitation until we\n> > -\t\t\t * figure out the exact capabilities of the hardware.\n> > +\t\t\t * Default viewfinder to 1280x720, capped to the maximum\n> > +\t\t\t * sensor resolution and aligned to the ImgU output\n> > +\t\t\t * constraints.\n>\n> This is done for both Viewfinder and VideoRecording right?\n>\n\nYes, I'll update the comment\n\n> > \t\t\t */\n> > -\t\t\tif (streams.find(&data->vfStream_) == streams.end()) {\n> > -\t\t\t\tLOG(IPU3, Error)\n> > -\t\t\t\t\t<< \"No stream available for requested role \"\n> > -\t\t\t\t\t<< role;\n> > -\t\t\t\tbreak;\n> > -\t\t\t}\n> > -\n> > -\t\t\tstream = &data->vfStream_;\n> > -\n> > -\t\t\t/*\n> > -\t\t\t * Align the default viewfinder size to the maximum\n> > -\t\t\t * available sensor resolution and to the IPU3\n> > -\t\t\t * alignment constraints.\n> > -\t\t\t */\n> > -\t\t\tconst Size &res = data->cio2_.sensor()->resolution();\n> > -\t\t\tunsigned int width = std::min(1280U, res.width);\n> > -\t\t\tunsigned int height = std::min(720U, res.height);\n> > -\t\t\tcfg.size = { width & ~7, height & ~3 };\n> > +\t\t\tcfg.size.width = std::min(1280U, sensorResolution.width);\n> > +\t\t\tcfg.size.height = std::min(720U, sensorResolution.height);\n> > +\t\t\tcfg.size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;\n> > +\t\t\tcfg.size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;\n> > +\t\t\tcfg.pixelFormat = formats::NV12;\n> > +\t\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\n> >\n> > \t\t\tbreak;\n> > \t\t}\n> > @@ -388,16 +350,10 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > \t\tdefault:\n> > \t\t\tLOG(IPU3, Error)\n> > \t\t\t\t<< \"Requested stream role not supported: \" << role;\n> > -\t\t\tbreak;\n>\n> I would keep the break as it is the style elsewhere in camera is to\n> terminate the default case with either break, return or continue.\n>\n\nThe code now looks like this\n\n\t\tdefault:\n\t\t\tLOG(IPU3, Error)\n\t\t\t\t<< \"Requested stream role not supported: \" << role;\n\t\t\tdelete config;\n\t\t\treturn nullptr;\n\nI think it's right\n\nThanks\n j\n\n> > -\t\t}\n> > -\n> > -\t\tif (!stream) {\n> > \t\t\tdelete config;\n> > \t\t\treturn nullptr;\n> > \t\t}\n> >\n> > -\t\tstreams.erase(stream);\n> > -\n> > \t\tconfig->addConfiguration(cfg);\n> > \t}\n> >\n> > --\n> > 2.27.0\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund","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 4925BBD792\n\tfor ;\n\tFri, 10 Jul 2020 06:55:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8EC6E61392;\n\tFri, 10 Jul 2020 08:55:22 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EFB2B61214\n\tfor ;\n\tFri, 10 Jul 2020 08:55:20 +0200 (CEST)","from uno.localdomain (host-95-245-128-189.retail.telecomitalia.it\n\t[95.245.128.189]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id CD58F100002;\n\tFri, 10 Jul 2020 06:55:19 +0000 (UTC)"],"Date":"Fri, 10 Jul 2020 08:58:54 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200710065854.3g5ntdslt3mjgxhe@uno.localdomain>","References":"<20200709084128.5316-1-jacopo@jmondi.org>\n\t<20200709084128.5316-3-jacopo@jmondi.org>\n\t<20200709125942.GG3875643@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200709125942.GG3875643@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v2 02/20] libcamera: ipu3: Remove\n\tstreams from generateConfiguration","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11311,"web_url":"https://patchwork.libcamera.org/comment/11311/","msgid":"<20200710082727.GB5964@pendragon.ideasonboard.com>","date":"2020-07-10T08:27:27","subject":"Re: [libcamera-devel] [PATCH v2 02/20] libcamera: ipu3: Remove\n\tstreams from generateConfiguration","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\nIsn't the usual practice to use \"libcamera: pipeline:\" as a prefix for\npipeline handler patches in the subject line ?\n\nOn Fri, Jul 10, 2020 at 08:58:54AM +0200, Jacopo Mondi wrote:\n> On Thu, Jul 09, 2020 at 02:59:42PM +0200, Niklas Söderlund wrote:\n> > On 2020-07-09 10:41:10 +0200, Jacopo Mondi wrote:\n> > > Remove stream assignment from the IPU3 pipeline handler\n> > > generateConfiguration() implementation.\n> > >\n> > > The function aims to provide a suitable default for the requested use\n> > > cases. Defer stream assignment to validation and only initialize sizes\n> > > and formats.\n> > >\n> > > Reviewed-by: Kieran Bingham \n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 104 ++++++++-------------------\n> > > 1 file changed, 30 insertions(+), 74 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index af51fb2d1718..85d21b4db046 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -31,6 +31,14 @@ 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 unsigned int IPU3_OUTPUT_MAX_WIDTH = 4480;\n> > > +static constexpr unsigned int IPU3_OUTPUT_MAX_HEIGHT = 34004;\n> >\n> > Just checking is 34004 the max height?\n> \n> It seems so\n> drivers/staging/media/ipu3/ipu3.h-#define IPU3_OUTPUT_MAX_HEIGHT 34004U\n\nA weird value, but why not.\n\n> > > +static const Size minIPU3OutputSize = { 2, 2 };\n> > > +static constexpr unsigned int IPU3_OUTPUT_WIDTH_ALIGN = 0x3f;\n> > > +static constexpr unsigned int IPU3_OUTPUT_HEIGHT_ALIGN = 0x3;\n\nI'd set these to 64 and 4, as the alignment constraints are multiple of\n4 and multiple of 4, and subtract 1 in the code.\n\nI would also rename the constants to use an IMGU prefix if they're\nspecific to the ImgU (those two seem to be, I don't know about the\nprevious ones).\n\n> > > +\n> > > class IPU3CameraData : public CameraData\n> > > {\n> > > public:\n> > > @@ -61,9 +69,6 @@ public:\n> > > \tconst std::vector &streams() { return streams_; }\n> > >\n> > > private:\n> > > -\tstatic constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> > > -\tstatic constexpr unsigned int IPU3_MAX_STREAMS = 3;\n> > > -\n> > > \tvoid assignStreams();\n> > > \tvoid adjustStream(StreamConfiguration &cfg, bool scale);\n> > >\n> > > @@ -293,94 +298,51 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > > {\n> > > \tIPU3CameraData *data = cameraData(camera);\n> > > \tIPU3CameraConfiguration *config = new IPU3CameraConfiguration(camera, data);\n> > > -\tstd::set streams = {\n> > > -\t\t&data->outStream_,\n> > > -\t\t&data->vfStream_,\n> > > -\t\t&data->rawStream_,\n> > > -\t};\n> > >\n> > > \tif (roles.empty())\n> > > \t\treturn config;\n> > >\n> > > +\tSize sensorResolution = data->cio2_.sensor()->resolution();\n> > > \tfor (const StreamRole role : roles) {\n> > > \t\tStreamConfiguration cfg = {};\n> > > -\t\tStream *stream = nullptr;\n> > > -\n> > > -\t\tcfg.pixelFormat = formats::NV12;\n> > >\n> > > \t\tswitch (role) {\n> > > \t\tcase StreamRole::StillCapture:\n> > > \t\t\t/*\n> > > -\t\t\t * Pick the output stream by default as the Viewfinder\n> > > -\t\t\t * and VideoRecording roles are not allowed on\n> > > -\t\t\t * the output stream.\n> > > -\t\t\t */\n> > > -\t\t\tif (streams.find(&data->outStream_) != streams.end()) {\n> > > -\t\t\t\tstream = &data->outStream_;\n> > > -\t\t\t} else if (streams.find(&data->vfStream_) != streams.end()) {\n> > > -\t\t\t\tstream = &data->vfStream_;\n> > > -\t\t\t} else {\n> > > -\t\t\t\tLOG(IPU3, Error)\n> > > -\t\t\t\t\t<< \"No stream available for requested role \"\n> > > -\t\t\t\t\t<< role;\n> > > -\t\t\t\tbreak;\n> > > -\t\t\t}\n> > > -\n> > > -\t\t\t/*\n> > > -\t\t\t * FIXME: Soraka: the maximum resolution reported by\n> > > -\t\t\t * both sensors (2592x1944 for ov5670 and 4224x3136 for\n> > > -\t\t\t * ov13858) are returned as default configurations but\n> > > -\t\t\t * they're not correctly processed by the ImgU.\n> > > -\t\t\t * Resolutions up tp 2560x1920 have been validated.\n> > > -\t\t\t *\n> > > -\t\t\t * \\todo Clarify ImgU alignment requirements.\n> > > +\t\t\t * Use the sensor resolution aligned to the ImgU\n> > > +\t\t\t * output constraints.\n> > > \t\t\t */\n> > > -\t\t\tcfg.size = { 2560, 1920 };\n> > > +\t\t\tcfg.size.width = std::min(sensorResolution.width,\n> > > +\t\t\t\t\t\t IPU3_OUTPUT_MAX_WIDTH);\n> > > +\t\t\tcfg.size.height = std::min(sensorResolution.height,\n> > > +\t\t\t\t\t\t IPU3_OUTPUT_MAX_HEIGHT);\n> > > +\t\t\tcfg.size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;\n> > > +\t\t\tcfg.size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;\n\nWould \"[PATCH] libcamera: geometry: Add helper functions to the Size\nclass\" help ?\n\n\t\t\tcfg.size = sensorResolution.boundedTo({IPU3_OUTPUT_MAX_WIDTH,\n\t\t\t\t\t\t\t IPU3_OUTPUT_MAX_HEIGHT})\n\t\t\t\t\t\t .alignedTo(IPU3_OUTPUT_WIDTH_ALIGN,\n\t\t\t\t\t\t\t IPU3_OUTPUT_HEIGHT_ALIGN);\n\nIPU3_OUTPUT_MAX_WIDTH and IPU3_OUTPUT_MAX_HEIGHT could become\nIPU3_OUTPUT_MAX_SIZE to simplify it further.\n\nReviewed-by: Laurent Pinchart \n\n> > > +\t\t\tcfg.pixelFormat = formats::NV12;\n> > > +\t\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\n> > >\n> > > \t\t\tbreak;\n> > >\n> > > \t\tcase StreamRole::StillCaptureRaw: {\n> > > -\t\t\tif (streams.find(&data->rawStream_) == streams.end()) {\n> > > -\t\t\t\tLOG(IPU3, Error)\n> > > -\t\t\t\t\t<< \"Multiple raw streams are not supported\";\n> > > -\t\t\t\tbreak;\n> > > -\t\t\t}\n> > > -\n> > > -\t\t\tstream = &data->rawStream_;\n> > > -\n> > > -\t\t\tcfg.size = data->cio2_.sensor()->resolution();\n> > > +\t\t\tcfg = data->cio2_.generateConfiguration(sensorResolution);\n> > > +\t\t\tcfg.bufferCount = 1;\n> >\n> > Setting bufferCount here is goes against the idea of having\n> > CIO2Device::generateConfiguration() control all the parameters of the\n> > RAW stream. I know there was some discussion if returning a\n> > StreamConfiguration from the CIO2Device is a good idea or not. I still\n> > think it's the best design but I'm open to change/discuss it.\n> \n> Ah, this was not intentional in first place to be honest.\n> \n> I would be happy to discuss (later) what is the best to return from\n> the CIO2 configuration, as I would have preferred not returning a\n> format, but that's oke for now.\n> \n> > But if we really want to modify parameters of it here I agree returning\n> > a StreamConfiguration here is confusing, double so as bufferCount is set\n> > to CIO2_BUFFER_COUNT and then changed to 1 here.\n> \n> What confused me is the pattern\n> \n> \t\tcfg.size = data->cio2_.sensor()->resolution();\n> \t\tcfg = data->cio2_.generateConfiguration(cfg.size);\n> \n> As using cfg.size just a place holder to pass it to the\n> generateConfiguration() function is not nice imho. But removing the\n> assignement of the whole configuration was not intentional, so I'll\n> fix it.\n> \n> > >\n> > > -\t\t\tcfg = data->cio2_.generateConfiguration(cfg.size);\n> > > \t\t\tbreak;\n> > > \t\t}\n> > >\n> > > \t\tcase StreamRole::Viewfinder:\n> > > \t\tcase StreamRole::VideoRecording: {\n> > > \t\t\t/*\n> > > -\t\t\t * We can't use the 'output' stream for viewfinder or\n> > > -\t\t\t * video capture roles.\n> > > -\t\t\t *\n> > > -\t\t\t * \\todo This is an artificial limitation until we\n> > > -\t\t\t * figure out the exact capabilities of the hardware.\n> > > +\t\t\t * Default viewfinder to 1280x720, capped to the maximum\n> > > +\t\t\t * sensor resolution and aligned to the ImgU output\n> > > +\t\t\t * constraints.\n> >\n> > This is done for both Viewfinder and VideoRecording right?\n> \n> Yes, I'll update the comment\n> \n> > > \t\t\t */\n> > > -\t\t\tif (streams.find(&data->vfStream_) == streams.end()) {\n> > > -\t\t\t\tLOG(IPU3, Error)\n> > > -\t\t\t\t\t<< \"No stream available for requested role \"\n> > > -\t\t\t\t\t<< role;\n> > > -\t\t\t\tbreak;\n> > > -\t\t\t}\n> > > -\n> > > -\t\t\tstream = &data->vfStream_;\n> > > -\n> > > -\t\t\t/*\n> > > -\t\t\t * Align the default viewfinder size to the maximum\n> > > -\t\t\t * available sensor resolution and to the IPU3\n> > > -\t\t\t * alignment constraints.\n> > > -\t\t\t */\n> > > -\t\t\tconst Size &res = data->cio2_.sensor()->resolution();\n> > > -\t\t\tunsigned int width = std::min(1280U, res.width);\n> > > -\t\t\tunsigned int height = std::min(720U, res.height);\n> > > -\t\t\tcfg.size = { width & ~7, height & ~3 };\n> > > +\t\t\tcfg.size.width = std::min(1280U, sensorResolution.width);\n> > > +\t\t\tcfg.size.height = std::min(720U, sensorResolution.height);\n> > > +\t\t\tcfg.size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;\n> > > +\t\t\tcfg.size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;\n> > > +\t\t\tcfg.pixelFormat = formats::NV12;\n> > > +\t\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\n> > >\n> > > \t\t\tbreak;\n> > > \t\t}\n> > > @@ -388,16 +350,10 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > > \t\tdefault:\n> > > \t\t\tLOG(IPU3, Error)\n> > > \t\t\t\t<< \"Requested stream role not supported: \" << role;\n> > > -\t\t\tbreak;\n> >\n> > I would keep the break as it is the style elsewhere in camera is to\n> > terminate the default case with either break, return or continue.\n> \n> The code now looks like this\n> \n> \t\tdefault:\n> \t\t\tLOG(IPU3, Error)\n> \t\t\t\t<< \"Requested stream role not supported: \" << role;\n> \t\t\tdelete config;\n> \t\t\treturn nullptr;\n> \n> I think it's right\n> \n> > > -\t\t}\n> > > -\n> > > -\t\tif (!stream) {\n> > > \t\t\tdelete config;\n> > > \t\t\treturn nullptr;\n> > > \t\t}\n> > >\n> > > -\t\tstreams.erase(stream);\n> > > -\n> > > \t\tconfig->addConfiguration(cfg);\n> > > \t}\n> > >","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 A95BFBD792\n\tfor ;\n\tFri, 10 Jul 2020 08:27:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3201D613B6;\n\tFri, 10 Jul 2020 10:27:35 +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 4EB4261396\n\tfor ;\n\tFri, 10 Jul 2020 10:27:34 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C3ADF2C0;\n\tFri, 10 Jul 2020 10:27:33 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"TgYYYN6M\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1594369653;\n\tbh=c7RfaDcCP7ZXlwvA7V3cjIyPi8ieW2d0N0tAj0Twthg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TgYYYN6MHwZ8GjKses787h6qaS+51HrdprsKdNugr8ESJaDqiTIfTzSPd2b1i1/tG\n\tPd58QulRDz1Ncc/hzg38grTdkmFz5pe5jDYcKo7z9+UDKoJ1DuQEYfDVNR+lvKe7M6\n\thr/igWr2P4Neu3TjW3Oqbal/9X9Fg6Y1f6B7akzo=","Date":"Fri, 10 Jul 2020 11:27:27 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200710082727.GB5964@pendragon.ideasonboard.com>","References":"<20200709084128.5316-1-jacopo@jmondi.org>\n\t<20200709084128.5316-3-jacopo@jmondi.org>\n\t<20200709125942.GG3875643@oden.dyn.berto.se>\n\t<20200710065854.3g5ntdslt3mjgxhe@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200710065854.3g5ntdslt3mjgxhe@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 02/20] libcamera: ipu3: Remove\n\tstreams from generateConfiguration","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]