[{"id":1167,"web_url":"https://patchwork.libcamera.org/comment/1167/","msgid":"<20190402073258.GD4805@pendragon.ideasonboard.com>","date":"2019-04-02T07:32:58","subject":"Re: [libcamera-devel] [PATCH v5 09/19] libcamera: ipu3: Set stream\n\tconfiguration from sensor","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 Tue, Mar 26, 2019 at 09:38:52AM +0100, Jacopo Mondi wrote:\n> Inspect all image sizes provided by the sensor and select the\n> biggest of them, associated with an image format code supported by the\n> CIO2 unit.\n\nThis isn't correct anymore, you don't inspect all sizes here, you use\nthe cached maximum size.\n\n\"While at it, replace the hardcoded numerical value for the number of\nbuffers with a named constexpr.\"\n\n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 35 +++++++++++++---------------\n> 1 file changed, 16 insertions(+), 19 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index d42c81273cc6..04cd02653711 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -5,6 +5,7 @@\n> * ipu3.cpp - Pipeline handler for Intel IPU3\n> */\n> \n> +#include \n> #include \n> #include \n> \n> @@ -94,6 +95,8 @@ private:\n> \t\tstd::pair maxSizes_;\n> \t};\n> \n> +\tstatic constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> +\n> \tIPU3CameraData *cameraData(const Camera *camera)\n> \t{\n> \t\treturn static_cast(\n> @@ -124,26 +127,20 @@ std::map\n> PipelineHandlerIPU3::streamConfiguration(Camera *camera,\n> \t\t\t\t\t std::set &streams)\n> {\n> -\tIPU3CameraData *data = cameraData(camera);\n> \tstd::map configs;\n> -\tV4L2SubdeviceFormat format = {};\n> -\n> -\t/*\n> -\t * FIXME: As of now, return the image format reported by the sensor.\n> -\t * In future good defaults should be provided for each stream.\n> -\t */\n> -\tif (data->sensor_->getFormat(0, &format)) {\n> -\t\tLOG(IPU3, Error) << \"Failed to create stream configurations\";\n> -\t\treturn configs;\n> -\t}\n> -\n> -\tStreamConfiguration config = {};\n> -\tconfig.width = format.width;\n> -\tconfig.height = format.height;\n> -\tconfig.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10;\n> -\tconfig.bufferCount = 4;\n> -\n> -\tconfigs[&data->stream_] = config;\n> +\tIPU3CameraData *data = cameraData(camera);\n\nI'd keep this above the configs variable declaration to make the diff\nmore readable (unless you think there's an advantage in this new order).\n\n> +\tStreamConfiguration *config = &configs[&data->stream_];\n> +\tSizeRange &maxRange = data->maxSizes_.second;\n> +\n> +\tconfig->width = maxRange.maxWidth;\n> +\tconfig->height = maxRange.maxHeight;\n> +\tconfig->pixelFormat = data->maxSizes_.first;\n> +\tconfig->bufferCount = IPU3_BUFFER_COUNT;\n> +\n> +\tLOG(IPU3, Debug)\n> +\t\t<< \"Stream format set to: \" << config->width << \"x\"\n\ns/to:/to/\n\n> +\t\t<< config->height << \"-0x\" << std::hex << std::setfill('0')\n> +\t\t<< std::setw(4) << config->pixelFormat;\n\nSomething to keep in mind for later, adding a Format class may be a good\nidea, to store pixel format, width and height. We should then give it a\ntoString() method. Or do you think it would make sense to add a\ntoString() method to StreamConfiguration and use it here ?\n\n> \treturn configs;\n> }","headers":{"Return-Path":"","Received":["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 273BD60DB3\n\tfor ;\n\tTue, 2 Apr 2019 09:33:17 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 928F02EC;\n\tTue, 2 Apr 2019 09:33:16 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1554190396;\n\tbh=Etab2UDMGpPOSZTFGDkm1KFTmgCOyFuD6QlnV1nfJ/k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KR4NWG1yVAXVHJHcEFb759Wfo6rytB+Z0SnWOk+JqSXH5c5/v1KVvgcU+/JkJorL/\n\tNbMwfonqDVmzBQCEyguURrFrJvpTsZfy9R7KF9OGOQFOrPLSmtDcDf71hjBHn34+qm\n\tEouLiIkdm+E5pzeuJvuEog7k1PqOeeZNkVkUzIJo=","Date":"Tue, 2 Apr 2019 10:32:58 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190402073258.GD4805@pendragon.ideasonboard.com>","References":"<20190326083902.26121-1-jacopo@jmondi.org>\n\t<20190326083902.26121-10-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190326083902.26121-10-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v5 09/19] libcamera: ipu3: Set stream\n\tconfiguration from sensor","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 02 Apr 2019 07:33:17 -0000"}},{"id":1173,"web_url":"https://patchwork.libcamera.org/comment/1173/","msgid":"<20190402082837.q2klsggzx2q327rm@uno.localdomain>","date":"2019-04-02T08:28:37","subject":"Re: [libcamera-devel] [PATCH v5 09/19] libcamera: ipu3: Set stream\n\tconfiguration from sensor","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, Apr 02, 2019 at 10:32:58AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Tue, Mar 26, 2019 at 09:38:52AM +0100, Jacopo Mondi wrote:\n> > Inspect all image sizes provided by the sensor and select the\n> > biggest of them, associated with an image format code supported by the\n> > CIO2 unit.\n>\n> This isn't correct anymore, you don't inspect all sizes here, you use\n> the cached maximum size.\n>\n> \"While at it, replace the hardcoded numerical value for the number of\n> buffers with a named constexpr.\"\n>\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 35 +++++++++++++---------------\n> > 1 file changed, 16 insertions(+), 19 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index d42c81273cc6..04cd02653711 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -5,6 +5,7 @@\n> > * ipu3.cpp - Pipeline handler for Intel IPU3\n> > */\n> >\n> > +#include \n> > #include \n> > #include \n> >\n> > @@ -94,6 +95,8 @@ private:\n> > \t\tstd::pair maxSizes_;\n> > \t};\n> >\n> > +\tstatic constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> > +\n> > \tIPU3CameraData *cameraData(const Camera *camera)\n> > \t{\n> > \t\treturn static_cast(\n> > @@ -124,26 +127,20 @@ std::map\n> > PipelineHandlerIPU3::streamConfiguration(Camera *camera,\n> > \t\t\t\t\t std::set &streams)\n> > {\n> > -\tIPU3CameraData *data = cameraData(camera);\n> > \tstd::map configs;\n> > -\tV4L2SubdeviceFormat format = {};\n> > -\n> > -\t/*\n> > -\t * FIXME: As of now, return the image format reported by the sensor.\n> > -\t * In future good defaults should be provided for each stream.\n> > -\t */\n> > -\tif (data->sensor_->getFormat(0, &format)) {\n> > -\t\tLOG(IPU3, Error) << \"Failed to create stream configurations\";\n> > -\t\treturn configs;\n> > -\t}\n> > -\n> > -\tStreamConfiguration config = {};\n> > -\tconfig.width = format.width;\n> > -\tconfig.height = format.height;\n> > -\tconfig.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10;\n> > -\tconfig.bufferCount = 4;\n> > -\n> > -\tconfigs[&data->stream_] = config;\n> > +\tIPU3CameraData *data = cameraData(camera);\n>\n> I'd keep this above the configs variable declaration to make the diff\n> more readable (unless you think there's an advantage in this new order).\n>\n\nYes, the declaration order follows reverse line lenght ordering. Diff\nmight be more confused a little, but what we care about is the end\nresult, isn't it?\n\n> > +\tStreamConfiguration *config = &configs[&data->stream_];\n> > +\tSizeRange &maxRange = data->maxSizes_.second;\n> > +\n> > +\tconfig->width = maxRange.maxWidth;\n> > +\tconfig->height = maxRange.maxHeight;\n> > +\tconfig->pixelFormat = data->maxSizes_.first;\n> > +\tconfig->bufferCount = IPU3_BUFFER_COUNT;\n> > +\n> > +\tLOG(IPU3, Debug)\n> > +\t\t<< \"Stream format set to: \" << config->width << \"x\"\n>\n> s/to:/to/\n>\n> > +\t\t<< config->height << \"-0x\" << std::hex << std::setfill('0')\n> > +\t\t<< std::setw(4) << config->pixelFormat;\n>\n> Something to keep in mind for later, adding a Format class may be a good\n> idea, to store pixel format, width and height. We should then give it a\n> toString() method. Or do you think it would make sense to add a\n> toString() method to StreamConfiguration and use it here ?\n\nTime, and more users, will tell. We'll need a Format class for sure,\nwe could add pretty printing there.\n\nThanks\n j\n>\n> > \treturn configs;\n> > }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["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 4A65060DB3\n\tfor ;\n\tTue, 2 Apr 2019 10:27:53 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id C37AF40017;\n\tTue, 2 Apr 2019 08:27:52 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 2 Apr 2019 10:28:37 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190402082837.q2klsggzx2q327rm@uno.localdomain>","References":"<20190326083902.26121-1-jacopo@jmondi.org>\n\t<20190326083902.26121-10-jacopo@jmondi.org>\n\t<20190402073258.GD4805@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"ud2znff7we3oh74o\"","Content-Disposition":"inline","In-Reply-To":"<20190402073258.GD4805@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v5 09/19] libcamera: ipu3: Set stream\n\tconfiguration from sensor","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 02 Apr 2019 08:27:53 -0000"}}]