[{"id":11047,"web_url":"https://patchwork.libcamera.org/comment/11047/","msgid":"<20200701162306.GD2399385@oden.dyn.berto.se>","date":"2020-07-01T16:23:06","subject":"Re: [libcamera-devel] [PATCH 02/15] libcamera: ipu3: Remove streams\n\tfrom 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-01 14:30:23 +0200, Jacopo Mondi wrote:\n> Remove stream assignement 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 assignement to validation and only assign sizes\n> and formats to stream configurations.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 93 ++++++++--------------------\n> 1 file changed, 27 insertions(+), 66 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 1bdad209de6e..97fc8b60c3cb 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -31,6 +31,9 @@ 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> +\n> class IPU3CameraData : public CameraData\n> {\n> public:\n> @@ -61,9 +64,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 +293,55 @@ 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> +\tunsigned int rawCount = 0;\n> +\tunsigned int outCount = 0;\n\nDoes it make sens to add check of number of streams here? In 7/15 you \nadd the same to validate() and validate is called at the and of \ngenerateConfiguration().\n\nOther then this open question I really like this patch!\n\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 * Use the sensor resolution adjusted to respect the\n> +\t\t\t * ImgU output alignement contraints.\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> +\t\t\tcfg.pixelFormat = formats::NV12;\n> +\t\t\tcfg.size = sensorResolution;\n> +\t\t\tcfg.size.width &= ~7;\n> +\t\t\tcfg.size.height &= ~3;\n> +\t\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\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 */\n> -\t\t\tcfg.size = { 2560, 1920 };\n> +\t\t\toutCount++;\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> +\t\t\tcfg = data->cio2_.generateConfiguration(sensorResolution);\n> +\t\t\tcfg.bufferCount = 1;\n> \n> -\t\t\tstream = &data->rawStream_;\n> +\t\t\trawCount++;\n> \n> -\t\t\tcfg.size = data->cio2_.sensor()->resolution();\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 */\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\tunsigned int width = std::min(1280U, sensorResolution.width);\n> +\t\t\tunsigned int height = std::min(720U, sensorResolution.height);\n> \t\t\tcfg.size = { width & ~7, height & ~3 };\n> +\t\t\tcfg.pixelFormat = formats::NV12;\n> +\t\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\n> +\n> +\t\t\toutCount++;\n> \n> \t\t\tbreak;\n> \t\t}\n> @@ -388,16 +349,16 @@ 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> +\t\t\tdelete config;\n> +\t\t\treturn nullptr;\n> \t\t}\n> \n> -\t\tif (!stream) {\n> +\t\tif (rawCount > 1 || outCount > 2) {\n> +\t\t\tLOG(IPU3, Error) << \"Invalid stream roles requested\";\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 75000BE905\n\tfor ;\n\tWed, 1 Jul 2020 16:23:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0E9F160C58;\n\tWed, 1 Jul 2020 18:23:10 +0200 (CEST)","from mail-lf1-x12a.google.com (mail-lf1-x12a.google.com\n\t[IPv6:2a00:1450:4864:20::12a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A7342609A9\n\tfor ;\n\tWed, 1 Jul 2020 18:23:08 +0200 (CEST)","by mail-lf1-x12a.google.com with SMTP id k15so14016685lfc.4\n\tfor ;\n\tWed, 01 Jul 2020 09:23:08 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tr15sm1974400ljd.130.2020.07.01.09.23.07\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 01 Jul 2020 09:23:07 -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=\"Lq0dpz9c\"; 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=Qs03Zv18ssHCZyubvEIqFa5IOHPiqGSiGqO658q0WLQ=;\n\tb=Lq0dpz9c7OThvsN0joMPp7L+w6EB/GnrUdaZaBWW9CKbJJ4RjxsVjiOLD/WzWCI2GW\n\thsIJK/1A8P9un0oCCVwm4bWO6PUwXWgN+qHYxRUGawQ+0G36pT0LOTAkVMIy68sl8l8A\n\trTe1okA+5X4Wb+0cWwOfdfz+lCWDnhBUS5LIcC3xTE3qedC2alGQzBdAG2aaC3t83Njk\n\tPvJQP/IlPIsc48MwjHzN/lvLRufVy77XoP90+ZJX+R9GdfGT5fXnubURsFYlHVDG3Ma3\n\t3TwfQT8Sp0/epDDa+SA38A7k2YZhdT8Zb9yT0by+UIaje3g6h0BmAiVLVkkZ0cWI7paT\n\tqL7A==","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=Qs03Zv18ssHCZyubvEIqFa5IOHPiqGSiGqO658q0WLQ=;\n\tb=ke1l1GPruXW2xyxx7HWUWpVqj24PrX6f/6697Et3R4OLsPdALfS89pQtlfYR2r9S4+\n\t3/J/RJ6R7wNIHYiRinlgX6khIns0l2U/IIwKuF1egpWqXVMXv3h09slAalAGL5sxODx0\n\t9ttnKkBbTLcQVxchi4qfCHI1GEx82bsJLdRFDKKybRw9EKlKA8pyQuNd889w7cSnmKIs\n\tYFLI8VFJaUJYf18gGZWP/d9xsybQtheEIBs5RjlPkTPn9EFQdPRggvgDdRltSH5PY921\n\tqZAoJnHc7YOPOKzWbQSKuuj542vfCHdJFZfHnAN2+RLxhn51RN7OeelAjaRscMRDmsPH\n\tv5tQ==","X-Gm-Message-State":"AOAM532f8ZA9SRlyHGUgrZVPIvYcKQwCv0+sjrT2WO5vYDbFyF1rZg4g\n\tE/+3bjAo+Xj3u1N5SypJLhVljQ==","X-Google-Smtp-Source":"ABdhPJz93Dj2fhkj/yn2jsZDr46oT4DLSni5VcWSL5RJ+f5LUxqlLMkLjYbC2xNl7YxKG6Dt3Etibg==","X-Received":"by 2002:a05:6512:2018:: with SMTP id\n\ta24mr15533445lfb.105.1593620587812; \n\tWed, 01 Jul 2020 09:23:07 -0700 (PDT)","Date":"Wed, 1 Jul 2020 18:23:06 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20200701162306.GD2399385@oden.dyn.berto.se>","References":"<20200701123036.51922-1-jacopo@jmondi.org>\n\t<20200701123036.51922-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200701123036.51922-3-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 02/15] libcamera: ipu3: Remove streams\n\tfrom 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":11064,"web_url":"https://patchwork.libcamera.org/comment/11064/","msgid":"<20200702073336.mhvshdicttcvpspv@uno.localdomain>","date":"2020-07-02T07:33:36","subject":"Re: [libcamera-devel] [PATCH 02/15] libcamera: ipu3: Remove streams\n\tfrom generateConfiguration","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Wed, Jul 01, 2020 at 06:23:06PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2020-07-01 14:30:23 +0200, Jacopo Mondi wrote:\n> > Remove stream assignement 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 assignement to validation and only assign sizes\n> > and formats to stream configurations.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 93 ++++++++--------------------\n> > 1 file changed, 27 insertions(+), 66 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 1bdad209de6e..97fc8b60c3cb 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -31,6 +31,9 @@ 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> > +\n> > class IPU3CameraData : public CameraData\n> > {\n> > public:\n> > @@ -61,9 +64,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 +293,55 @@ 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> > +\tunsigned int rawCount = 0;\n> > +\tunsigned int outCount = 0;\n>\n> Does it make sens to add check of number of streams here? In 7/15 you\n> add the same to validate() and validate is called at the and of\n> generateConfiguration().\n\nIt's a bit of a repetetion, I agree.\n\nShould we check for the status returned by validate() then ?\n\n>\n> Other then this open question I really like this patch!\n>\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 * Use the sensor resolution adjusted to respect the\n> > +\t\t\t * ImgU output alignement contraints.\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> > +\t\t\tcfg.pixelFormat = formats::NV12;\n> > +\t\t\tcfg.size = sensorResolution;\n> > +\t\t\tcfg.size.width &= ~7;\n> > +\t\t\tcfg.size.height &= ~3;\n> > +\t\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\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 */\n> > -\t\t\tcfg.size = { 2560, 1920 };\n> > +\t\t\toutCount++;\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> > +\t\t\tcfg = data->cio2_.generateConfiguration(sensorResolution);\n> > +\t\t\tcfg.bufferCount = 1;\n> >\n> > -\t\t\tstream = &data->rawStream_;\n> > +\t\t\trawCount++;\n> >\n> > -\t\t\tcfg.size = data->cio2_.sensor()->resolution();\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 */\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\tunsigned int width = std::min(1280U, sensorResolution.width);\n> > +\t\t\tunsigned int height = std::min(720U, sensorResolution.height);\n> > \t\t\tcfg.size = { width & ~7, height & ~3 };\n> > +\t\t\tcfg.pixelFormat = formats::NV12;\n> > +\t\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\n> > +\n> > +\t\t\toutCount++;\n> >\n> > \t\t\tbreak;\n> > \t\t}\n> > @@ -388,16 +349,16 @@ 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> > +\t\t\tdelete config;\n> > +\t\t\treturn nullptr;\n> > \t\t}\n> >\n> > -\t\tif (!stream) {\n> > +\t\tif (rawCount > 1 || outCount > 2) {\n> > +\t\t\tLOG(IPU3, Error) << \"Invalid stream roles requested\";\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 BB833BE905\n\tfor ;\n\tThu, 2 Jul 2020 07:30:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4795B60C58;\n\tThu, 2 Jul 2020 09:30:07 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6159E603AE\n\tfor ;\n\tThu, 2 Jul 2020 09:30:06 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 57E4524000E;\n\tThu, 2 Jul 2020 07:30:04 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Thu, 2 Jul 2020 09:33:36 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200702073336.mhvshdicttcvpspv@uno.localdomain>","References":"<20200701123036.51922-1-jacopo@jmondi.org>\n\t<20200701123036.51922-3-jacopo@jmondi.org>\n\t<20200701162306.GD2399385@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200701162306.GD2399385@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH 02/15] libcamera: ipu3: Remove streams\n\tfrom 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":11069,"web_url":"https://patchwork.libcamera.org/comment/11069/","msgid":"<20200702073950.GB2807983@oden.dyn.berto.se>","date":"2020-07-02T07:39:50","subject":"Re: [libcamera-devel] [PATCH 02/15] libcamera: ipu3: Remove streams\n\tfrom 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\nOn 2020-07-02 09:33:36 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Wed, Jul 01, 2020 at 06:23:06PM +0200, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> >\n> > Thanks for your work.\n> >\n> > On 2020-07-01 14:30:23 +0200, Jacopo Mondi wrote:\n> > > Remove stream assignement 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 assignement to validation and only assign sizes\n> > > and formats to stream configurations.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 93 ++++++++--------------------\n> > > 1 file changed, 27 insertions(+), 66 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 1bdad209de6e..97fc8b60c3cb 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -31,6 +31,9 @@ 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> > > +\n> > > class IPU3CameraData : public CameraData\n> > > {\n> > > public:\n> > > @@ -61,9 +64,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 +293,55 @@ 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> > > +\tunsigned int rawCount = 0;\n> > > +\tunsigned int outCount = 0;\n> >\n> > Does it make sens to add check of number of streams here? In 7/15 you\n> > add the same to validate() and validate is called at the and of\n> > generateConfiguration().\n> \n> It's a bit of a repetetion, I agree.\n> \n> Should we check for the status returned by validate() then ?\n\nI think that would be nicer as we then collect all checks on the \nconfiguration in validate() while at the same time guarantee the \nconfiguration returned from generateConfiguration() is OK.\n\n> \n> >\n> > Other then this open question I really like this patch!\n> >\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 * Use the sensor resolution adjusted to respect the\n> > > +\t\t\t * ImgU output alignement contraints.\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> > > +\t\t\tcfg.pixelFormat = formats::NV12;\n> > > +\t\t\tcfg.size = sensorResolution;\n> > > +\t\t\tcfg.size.width &= ~7;\n> > > +\t\t\tcfg.size.height &= ~3;\n> > > +\t\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\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 */\n> > > -\t\t\tcfg.size = { 2560, 1920 };\n> > > +\t\t\toutCount++;\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> > > +\t\t\tcfg = data->cio2_.generateConfiguration(sensorResolution);\n> > > +\t\t\tcfg.bufferCount = 1;\n> > >\n> > > -\t\t\tstream = &data->rawStream_;\n> > > +\t\t\trawCount++;\n> > >\n> > > -\t\t\tcfg.size = data->cio2_.sensor()->resolution();\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 */\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\tunsigned int width = std::min(1280U, sensorResolution.width);\n> > > +\t\t\tunsigned int height = std::min(720U, sensorResolution.height);\n> > > \t\t\tcfg.size = { width & ~7, height & ~3 };\n> > > +\t\t\tcfg.pixelFormat = formats::NV12;\n> > > +\t\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\n> > > +\n> > > +\t\t\toutCount++;\n> > >\n> > > \t\t\tbreak;\n> > > \t\t}\n> > > @@ -388,16 +349,16 @@ 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> > > +\t\t\tdelete config;\n> > > +\t\t\treturn nullptr;\n> > > \t\t}\n> > >\n> > > -\t\tif (!stream) {\n> > > +\t\tif (rawCount > 1 || outCount > 2) {\n> > > +\t\t\tLOG(IPU3, Error) << \"Invalid stream roles requested\";\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 65316BE905\n\tfor ;\n\tThu, 2 Jul 2020 07:39:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 342C560C53;\n\tThu, 2 Jul 2020 09:39:54 +0200 (CEST)","from mail-lj1-x230.google.com (mail-lj1-x230.google.com\n\t[IPv6:2a00:1450:4864:20::230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4EF53603AE\n\tfor ;\n\tThu, 2 Jul 2020 09:39:52 +0200 (CEST)","by mail-lj1-x230.google.com with SMTP id b25so26956419ljp.6\n\tfor ;\n\tThu, 02 Jul 2020 00:39:52 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\ta22sm3039440lfg.96.2020.07.02.00.39.50\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 02 Jul 2020 00:39:50 -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=\"jF9uGK3w\"; 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=780/TaI37v3j5PwpLxYfUsKNaFls+DmpK25e9hP/UNg=;\n\tb=jF9uGK3wfwA6adk5+GcKJ1YNk2tJI+yTT4oee1hftZzXdOxvVNRYrY4bqOS1bDu4tC\n\tXly+BFggou+xHQ6sw5ARd73QQDCrzjyMeUQEagwNv4BLtw6jKDGwxNsXvpNgbvXhLxdO\n\tWd0g/2Z1YXobG5pvc8zaHLkRwt1RAJWQ2lH5mzVQuCdRnwVYsq2lnN9nTkJ+N68x/FlN\n\tOqWEbvQG+RalnRvB2DSUzkWp4vwTak01C5E4Pjo8pi6dSwDhkeAoLfBSjapUSoEgyzOy\n\tOryJtJUGXD72ZT0ESJPHeakNcn17xwrtIR9lyVAsIx9Jv0axOQ+ubtoke0fp9HVYcagX\n\t1zkg==","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=780/TaI37v3j5PwpLxYfUsKNaFls+DmpK25e9hP/UNg=;\n\tb=lx9mu6ee4qSXw000dqRG/at9km4QnIFUWcvHiANh/PVcqRv2/px8kuMScfGk7LRdmi\n\ty+nt2OW/c7uXxWWXa+kfCa5x2h1Kqax+oGX8WKvuOjtmu35dfs3Enl7WOeCePFzzzIH9\n\tMmHZvH1PsbwbcMrBNzm0KcDP7Ug1Nk4WIQlTLSun8OU4bMZncZYpjna63ZaEXNLKenZG\n\tDcm9AdoaFsomZ78g+zucId/Gnxk7dkgWa1j/6E43dfnMAyC6S7V++OSDWp7ZbhQfG8mV\n\tA3/Kq0hQ8p8+W00UW4FlvxNtlisvammSg6TqTZTGHyPfhycnI4U0OazXrxrQl3UeViMz\n\txDrQ==","X-Gm-Message-State":"AOAM531gorB2fLpIbioaLOq9Bu0ZnB36l9U9MYKpzXEpI46OBpBYmNh0\n\tUjG5RFfEuRtSzo+5w4TpmmjPBw==","X-Google-Smtp-Source":"ABdhPJxcOQ2jeI8Z6eMk3+lPTg6yA5FQIC4URv1RfuCziCJGQ6kNcTdmLLDcvFcdTx2id5iMjA9aRQ==","X-Received":"by 2002:a2e:6a05:: with SMTP id\n\tf5mr15834796ljc.385.1593675591378; \n\tThu, 02 Jul 2020 00:39:51 -0700 (PDT)","Date":"Thu, 2 Jul 2020 09:39:50 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20200702073950.GB2807983@oden.dyn.berto.se>","References":"<20200701123036.51922-1-jacopo@jmondi.org>\n\t<20200701123036.51922-3-jacopo@jmondi.org>\n\t<20200701162306.GD2399385@oden.dyn.berto.se>\n\t<20200702073336.mhvshdicttcvpspv@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200702073336.mhvshdicttcvpspv@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 02/15] libcamera: ipu3: Remove streams\n\tfrom 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":11076,"web_url":"https://patchwork.libcamera.org/comment/11076/","msgid":"<9b4f9945-8917-bfb4-3a41-622ebd649bac@ideasonboard.com>","date":"2020-07-02T14:04:44","subject":"Re: [libcamera-devel] [PATCH 02/15] libcamera: ipu3: Remove streams\n\tfrom generateConfiguration","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 01/07/2020 13:30, Jacopo Mondi wrote:\n> Remove stream assignement 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 assignement to validation and only assign sizes\n> and formats to stream configurations.\n\ns/assignement/assignment/ in two locations above\n\n\nExcellent, this sounds like exactly what will be needed as part of the\nuse of empty configurations for the Android HAL multi-stream work.\n\nIs all the code being removed already in validate()?\nI see an 'assignStreams()' there so I guess that covers it.\n\nReviewed-by: Kieran Bingham \n\nTrivial comment below but no blocker, and I saw Niklas had a couple of\nminor comments too, but those aside, I'll see how the rest of this\nseries affects the HAL multi-stream work.\n\n--\nKieran\n\n\n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 93 ++++++++--------------------\n> 1 file changed, 27 insertions(+), 66 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 1bdad209de6e..97fc8b60c3cb 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -31,6 +31,9 @@ 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> +\n> class IPU3CameraData : public CameraData\n> {\n> public:\n> @@ -61,9 +64,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 +293,55 @@ 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> +\tunsigned int rawCount = 0;\n> +\tunsigned int outCount = 0;\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 * Use the sensor resolution adjusted to respect the\n> +\t\t\t * ImgU output alignement contraints.\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> +\t\t\tcfg.pixelFormat = formats::NV12;\n> +\t\t\tcfg.size = sensorResolution;\n> +\t\t\tcfg.size.width &= ~7;\n> +\t\t\tcfg.size.height &= ~3;\n\nHrm ... do we need some align macros to make this more readable?\nNot necessarily required for this patch though.\n\n> +\t\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\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 */\n> -\t\t\tcfg.size = { 2560, 1920 };\n> +\t\t\toutCount++;\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> +\t\t\tcfg = data->cio2_.generateConfiguration(sensorResolution);\n> +\t\t\tcfg.bufferCount = 1;\n> \n> -\t\t\tstream = &data->rawStream_;\n> +\t\t\trawCount++;\n> \n> -\t\t\tcfg.size = data->cio2_.sensor()->resolution();\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 */\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\tunsigned int width = std::min(1280U, sensorResolution.width);\n> +\t\t\tunsigned int height = std::min(720U, sensorResolution.height);\n> \t\t\tcfg.size = { width & ~7, height & ~3 };\n> +\t\t\tcfg.pixelFormat = formats::NV12;\n> +\t\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\n> +\n> +\t\t\toutCount++;\n> \n> \t\t\tbreak;\n> \t\t}\n> @@ -388,16 +349,16 @@ 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> +\t\t\tdelete config;\n> +\t\t\treturn nullptr;\n> \t\t}\n> \n> -\t\tif (!stream) {\n> +\t\tif (rawCount > 1 || outCount > 2) {\n> +\t\t\tLOG(IPU3, Error) << \"Invalid stream roles requested\";\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>","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 73CDCBFFE2\n\tfor ;\n\tThu, 2 Jul 2020 14:04:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E8A7360C56;\n\tThu, 2 Jul 2020 16:04:49 +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 63AD3603B3\n\tfor ;\n\tThu, 2 Jul 2020 16:04:48 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C0C39293;\n\tThu, 2 Jul 2020 16:04:47 +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=\"jtkyrl0F\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593698687;\n\tbh=oLudGTdDc7muqv/cESSBvhPUht2BbtCHaE76Q+7RzHw=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=jtkyrl0FZ7C99WOFWWCYHDDH/JBL5kC2A9mZGEgBB4kVsrgxyQNTesnXvYKDHy6uo\n\t5GBxTudqy9XmK9MWdbmm+kNo6yVXmkDs2fB3BJsHAeSReGnrtyV8xEK96bbJoDvx/H\n\tfkkaw3gK1JH4MP+eem+Hfn9QWCz5sYd9GXOXrWj0=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20200701123036.51922-1-jacopo@jmondi.org>\n\t<20200701123036.51922-3-jacopo@jmondi.org>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<9b4f9945-8917-bfb4-3a41-622ebd649bac@ideasonboard.com>","Date":"Thu, 2 Jul 2020 15:04:44 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.8.0","MIME-Version":"1.0","In-Reply-To":"<20200701123036.51922-3-jacopo@jmondi.org>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 02/15] libcamera: ipu3: Remove streams\n\tfrom 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","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]