[{"id":1052,"web_url":"https://patchwork.libcamera.org/comment/1052/","msgid":"<20190313173224.GP4722@pendragon.ideasonboard.com>","date":"2019-03-13T17:32:24","subject":"Re: [libcamera-devel] [PATCH v2 03/14] libcamera: ipu3: Get default\n\timage sizes 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 12, 2019 at 01:12:31PM +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> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 94 ++++++++++++++++++++++++----\n> 1 file changed, 82 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 55489c31df2d..0f18e4692e77 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -6,8 +6,11 @@\n> */\n> \n> #include \n> +#include \n\nAlphabetical order please.\n\n> #include \n> \n> +#include \n> +\n> #include \n> #include \n> #include \n> @@ -72,12 +75,16 @@ private:\n> \t\tStream stream_;\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> \t\t\tPipelineHandler::cameraData(camera));\n> \t}\n> \n> +\tint mediaBusToCIO2Format(unsigned int code);\n\ns/;/const ;/\n\n> +\n> \tvoid registerCameras();\n> \n> \tstd::shared_ptr cio2_;\n> @@ -102,26 +109,45 @@ 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> +\tIPU3CameraData *data = cameraData(camera);\n> +\tV4L2Subdevice *sensor = data->sensor_;\n> +\tStreamConfiguration *config = &configs[&data->stream_];\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 * Make sure the sensor produces a raw format compatible with the\n> +\t * CIO2 and use the largest image size the sensor provides.\n> \t */\n> -\tif (data->sensor_->getFormat(0, &format)) {\n> -\t\tLOG(IPU3, Error) << \"Failed to create stream configurations\";\n> +\tconst SubdevFormatEnum formats = sensor->formats(0);\n> +\tfor (auto it = formats.begin(); it != formats.end(); ++it) {\n> +\t\tint cio2Code = mediaBusToCIO2Format(it->first);\n> +\t\tif (cio2Code == -EINVAL)\n> +\t\t\tcontinue;\n> +\n> +\t\tfor (const SizeRange &range : it->second) {\n> +\t\t\tif (range.maxWidth <= config->width ||\n> +\t\t\t range.maxHeight <= config->height)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tconfig->width = range.maxWidth;\n> +\t\t\tconfig->height = range.maxHeight;\n> +\t\t\tconfig->pixelFormat = cio2Code;\n> +\t\t}\n\nDidn't we decide to use the area instead of the width and height ?\n\n> +\t}\n> +\n> +\t/* If not suitable format has been found, return an empty config. */\n> +\tif (!config->pixelFormat) {\n\nThis relies on config being zeroed when constructed, which isn't\nguaranteed as far as I can tell.\n\n> +\t\tLOG(IPU3, Error) << \"Sensor image format not supported\";\n> +\n> \t\treturn configs;\n> \t}\n\nShouldn't this be done at match time and cached ? We shouldn't create a\ncamera whose sensor can't provide a supported format.\n\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> +\tconfig->bufferCount = IPU3_BUFFER_COUNT;\n> \n> -\tconfigs[&data->stream_] = config;\n> +\tLOG(IPU3, Debug)\n> +\t\t<< \"Stream format set to = (\" << config->width << \"x\"\n> +\t\t<< config->height << \") - 0x\" << std::hex << std::setfill('0')\n> +\t\t<< std::setw(8) << config->pixelFormat;\n> \n> \treturn configs;\n> }\n> @@ -327,6 +353,50 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> \treturn true;\n> }\n> \n> +int PipelineHandlerIPU3::mediaBusToCIO2Format(unsigned int code)\n> +{\n> +\tswitch(code) {\n> +\tcase MEDIA_BUS_FMT_SBGGR8_1X8:\n> +\tcase MEDIA_BUS_FMT_SBGGR10_1X10:\n> +\tcase MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE:\n> +\tcase MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE:\n> +\tcase MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE:\n> +\tcase MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE:\n> +\tcase MEDIA_BUS_FMT_SBGGR10_ALAW8_1X8:\n> +\tcase MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8:\n> +\tcase MEDIA_BUS_FMT_SBGGR12_1X12:\n> +\tcase MEDIA_BUS_FMT_SBGGR14_1X14:\n> +\tcase MEDIA_BUS_FMT_SBGGR16_1X16:\n> +\t\treturn V4L2_PIX_FMT_IPU3_SBGGR10;\n> +\tcase MEDIA_BUS_FMT_SGBRG8_1X8:\n> +\tcase MEDIA_BUS_FMT_SGBRG10_1X10:\n> +\tcase MEDIA_BUS_FMT_SGBRG10_ALAW8_1X8:\n> +\tcase MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8:\n> +\tcase MEDIA_BUS_FMT_SGBRG12_1X12:\n> +\tcase MEDIA_BUS_FMT_SGBRG14_1X14:\n> +\tcase MEDIA_BUS_FMT_SGBRG16_1X16:\n> +\t\treturn V4L2_PIX_FMT_IPU3_SGBRG10;\n> +\tcase MEDIA_BUS_FMT_SGRBG8_1X8:\n> +\tcase MEDIA_BUS_FMT_SGRBG10_1X10:\n> +\tcase MEDIA_BUS_FMT_SGRBG10_ALAW8_1X8:\n> +\tcase MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8:\n> +\tcase MEDIA_BUS_FMT_SGRBG12_1X12:\n> +\tcase MEDIA_BUS_FMT_SGRBG14_1X14:\n> +\tcase MEDIA_BUS_FMT_SGRBG16_1X16:\n> +\t\treturn V4L2_PIX_FMT_IPU3_SGRBG10;\n> +\tcase MEDIA_BUS_FMT_SRGGB8_1X8:\n> +\tcase MEDIA_BUS_FMT_SRGGB10_1X10:\n> +\tcase MEDIA_BUS_FMT_SRGGB10_ALAW8_1X8:\n> +\tcase MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8:\n> +\tcase MEDIA_BUS_FMT_SRGGB12_1X12:\n> +\tcase MEDIA_BUS_FMT_SRGGB14_1X14:\n> +\tcase MEDIA_BUS_FMT_SRGGB16_1X16:\n> +\t\treturn V4L2_PIX_FMT_IPU3_SRGGB10;\n\nDoes the CIO2 really support all these formats, especially the ALAW8,\nDPCM8 and > 10bpp formats ?\n\n> +\tdefault:\n> +\t\treturn -EINVAL;\n> +\t}\n> +}\n> +\n> /*\n> * Cameras are created associating an image sensor (represented by a\n> * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7A6FC600F9\n\tfor ;\n\tWed, 13 Mar 2019 18:32:32 +0100 (CET)","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 C427E5AA;\n\tWed, 13 Mar 2019 18:32:31 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1552498352;\n\tbh=xgaMTj8j4nr4Hc/j8/Pr6Vqh5rvng5SWSd8ELyQVPDU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=f70+ThnkY4FJWJdOUSk6OYXwxEIEvqPmGlgCx4FbapQB7bh2VjUlcPQ7kD+p98gCU\n\tgjei4uFRsIsRT+UEpdDD31nIL+b7DUvescwZQXWzPMz+otUawIr+zMmVN2d1Nqcwa2\n\tIjhQH8Y3C+xvvGosJs/ci7Hvp/7pUwx9YWCLETsw=","Date":"Wed, 13 Mar 2019 19:32:24 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190313173224.GP4722@pendragon.ideasonboard.com>","References":"<20190312121242.2253-1-jacopo@jmondi.org>\n\t<20190312121242.2253-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190312121242.2253-4-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 03/14] libcamera: ipu3: Get default\n\timage sizes 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":"Wed, 13 Mar 2019 17:32:32 -0000"}},{"id":1054,"web_url":"https://patchwork.libcamera.org/comment/1054/","msgid":"<20190314095238.jmgci5zr4zti4t4o@uno.localdomain>","date":"2019-03-14T09:52:38","subject":"Re: [libcamera-devel] [PATCH v2 03/14] libcamera: ipu3: Get default\n\timage sizes 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 Wed, Mar 13, 2019 at 07:32:24PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Tue, Mar 12, 2019 at 01:12:31PM +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> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 94 ++++++++++++++++++++++++----\n> > 1 file changed, 82 insertions(+), 12 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 55489c31df2d..0f18e4692e77 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -6,8 +6,11 @@\n> > */\n> >\n> > #include \n> > +#include \n>\n> Alphabetical order please.\n>\n\nTrivial, sorry\n\n> > #include \n> >\n> > +#include \n> > +\n> > #include \n> > #include \n> > #include \n> > @@ -72,12 +75,16 @@ private:\n> > \t\tStream stream_;\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> > \t\t\tPipelineHandler::cameraData(camera));\n> > \t}\n> >\n> > +\tint mediaBusToCIO2Format(unsigned int code);\n>\n> s/;/const ;/\n\nAck\n\n>\n> > +\n> > \tvoid registerCameras();\n> >\n> > \tstd::shared_ptr cio2_;\n> > @@ -102,26 +109,45 @@ 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> > +\tIPU3CameraData *data = cameraData(camera);\n> > +\tV4L2Subdevice *sensor = data->sensor_;\n> > +\tStreamConfiguration *config = &configs[&data->stream_];\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 * Make sure the sensor produces a raw format compatible with the\n> > +\t * CIO2 and use the largest image size the sensor provides.\n> > \t */\n> > -\tif (data->sensor_->getFormat(0, &format)) {\n> > -\t\tLOG(IPU3, Error) << \"Failed to create stream configurations\";\n> > +\tconst SubdevFormatEnum formats = sensor->formats(0);\n> > +\tfor (auto it = formats.begin(); it != formats.end(); ++it) {\n> > +\t\tint cio2Code = mediaBusToCIO2Format(it->first);\n> > +\t\tif (cio2Code == -EINVAL)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tfor (const SizeRange &range : it->second) {\n> > +\t\t\tif (range.maxWidth <= config->width ||\n> > +\t\t\t range.maxHeight <= config->height)\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\tconfig->width = range.maxWidth;\n> > +\t\t\tconfig->height = range.maxHeight;\n> > +\t\t\tconfig->pixelFormat = cio2Code;\n> > +\t\t}\n>\n> Didn't we decide to use the area instead of the width and height ?\n>\n\nI'm using the area when looking for the best size approximations, but\nhere I'm looking for the largest available resolution, and thus I\nwould like to have both width and height larger than the default one.\n\nI understand comparing area would work the same and catch some corner\ncases I would miss here. I'll fix.\n\n> > +\t}\n> > +\n> > +\t/* If not suitable format has been found, return an empty config. */\n> > +\tif (!config->pixelFormat) {\n>\n> This relies on config being zeroed when constructed, which isn't\n> guaranteed as far as I can tell.\n>\n\nI assume it got zeroed by Camera, but that seems not the be the case.\nI'll fix this too.\n\n> > +\t\tLOG(IPU3, Error) << \"Sensor image format not supported\";\n> > +\n> > \t\treturn configs;\n> > \t}\n>\n> Shouldn't this be done at match time and cached ? We shouldn't create a\n> camera whose sensor can't provide a supported format.\n>\n\nThat would make sense at camera registration time, yes, so I could\nskip this.\n\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> > +\tconfig->bufferCount = IPU3_BUFFER_COUNT;\n> >\n> > -\tconfigs[&data->stream_] = config;\n> > +\tLOG(IPU3, Debug)\n> > +\t\t<< \"Stream format set to = (\" << config->width << \"x\"\n> > +\t\t<< config->height << \") - 0x\" << std::hex << std::setfill('0')\n> > +\t\t<< std::setw(8) << config->pixelFormat;\n> >\n> > \treturn configs;\n> > }\n> > @@ -327,6 +353,50 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> > \treturn true;\n> > }\n> >\n> > +int PipelineHandlerIPU3::mediaBusToCIO2Format(unsigned int code)\n> > +{\n> > +\tswitch(code) {\n> > +\tcase MEDIA_BUS_FMT_SBGGR8_1X8:\n> > +\tcase MEDIA_BUS_FMT_SBGGR10_1X10:\n> > +\tcase MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE:\n> > +\tcase MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE:\n> > +\tcase MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE:\n> > +\tcase MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE:\n> > +\tcase MEDIA_BUS_FMT_SBGGR10_ALAW8_1X8:\n> > +\tcase MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8:\n> > +\tcase MEDIA_BUS_FMT_SBGGR12_1X12:\n> > +\tcase MEDIA_BUS_FMT_SBGGR14_1X14:\n> > +\tcase MEDIA_BUS_FMT_SBGGR16_1X16:\n> > +\t\treturn V4L2_PIX_FMT_IPU3_SBGGR10;\n> > +\tcase MEDIA_BUS_FMT_SGBRG8_1X8:\n> > +\tcase MEDIA_BUS_FMT_SGBRG10_1X10:\n> > +\tcase MEDIA_BUS_FMT_SGBRG10_ALAW8_1X8:\n> > +\tcase MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8:\n> > +\tcase MEDIA_BUS_FMT_SGBRG12_1X12:\n> > +\tcase MEDIA_BUS_FMT_SGBRG14_1X14:\n> > +\tcase MEDIA_BUS_FMT_SGBRG16_1X16:\n> > +\t\treturn V4L2_PIX_FMT_IPU3_SGBRG10;\n> > +\tcase MEDIA_BUS_FMT_SGRBG8_1X8:\n> > +\tcase MEDIA_BUS_FMT_SGRBG10_1X10:\n> > +\tcase MEDIA_BUS_FMT_SGRBG10_ALAW8_1X8:\n> > +\tcase MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8:\n> > +\tcase MEDIA_BUS_FMT_SGRBG12_1X12:\n> > +\tcase MEDIA_BUS_FMT_SGRBG14_1X14:\n> > +\tcase MEDIA_BUS_FMT_SGRBG16_1X16:\n> > +\t\treturn V4L2_PIX_FMT_IPU3_SGRBG10;\n> > +\tcase MEDIA_BUS_FMT_SRGGB8_1X8:\n> > +\tcase MEDIA_BUS_FMT_SRGGB10_1X10:\n> > +\tcase MEDIA_BUS_FMT_SRGGB10_ALAW8_1X8:\n> > +\tcase MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8:\n> > +\tcase MEDIA_BUS_FMT_SRGGB12_1X12:\n> > +\tcase MEDIA_BUS_FMT_SRGGB14_1X14:\n> > +\tcase MEDIA_BUS_FMT_SRGGB16_1X16:\n> > +\t\treturn V4L2_PIX_FMT_IPU3_SRGGB10;\n>\n> Does the CIO2 really support all these formats, especially the ALAW8,\n> DPCM8 and > 10bpp formats ?\n>\n\nNo idea. I intentionally put all v4l2 defined raw formats here to get\nfeedback. I'm fine restricting this to the most common 1x8 1x10 and\n1x12 formats (or just drop the exotic ALAW and DPCM ones).\n\nThanks\n j\n\n\n> > +\tdefault:\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +}\n> > +\n> > /*\n> > * Cameras are created associating an image sensor (represented by a\n> > * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E9331600FD\n\tfor ;\n\tThu, 14 Mar 2019 10:52:01 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 6A94FE0003;\n\tThu, 14 Mar 2019 09:52:01 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 14 Mar 2019 10:52:38 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190314095238.jmgci5zr4zti4t4o@uno.localdomain>","References":"<20190312121242.2253-1-jacopo@jmondi.org>\n\t<20190312121242.2253-4-jacopo@jmondi.org>\n\t<20190313173224.GP4722@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"b6vd6a6p5qcgwuwh\"","Content-Disposition":"inline","In-Reply-To":"<20190313173224.GP4722@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 03/14] libcamera: ipu3: Get default\n\timage sizes 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":"Thu, 14 Mar 2019 09:52:02 -0000"}}]