[{"id":1086,"web_url":"https://patchwork.libcamera.org/comment/1086/","msgid":"<20190321095418.GJ4615@pendragon.ideasonboard.com>","date":"2019-03-21T09:54:18","subject":"Re: [libcamera-devel] [PATCH v4 07/31] libcamera: ipu3: Propagate\n\timage format","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 Wed, Mar 20, 2019 at 05:30:31PM +0100, Jacopo Mondi wrote:\n> Apply the requested image format to the sensor device, and apply the\n> adjusted one to the CIO2 device, the ImgU subdevice and its input and\n> output video devices.\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  src/libcamera/pipeline/ipu3/ipu3.cpp | 220 +++++++++++++++++++++++----\n>  1 file changed, 189 insertions(+), 31 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 26fc56a76eb1..2975c218f6c9 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -16,6 +16,7 @@\n>  #include <libcamera/stream.h>\n>  \n>  #include \"device_enumerator.h\"\n> +#include \"geometry.h\"\n>  #include \"log.h\"\n>  #include \"media_device.h\"\n>  #include \"pipeline_handler.h\"\n> @@ -30,6 +31,11 @@ LOG_DEFINE_CATEGORY(IPU3)\n>  class ImgUDevice\n>  {\n>  public:\n> +\tstatic constexpr unsigned int PAD_INPUT = 0;\n> +\tstatic constexpr unsigned int PAD_OUTPUT = 2;\n> +\tstatic constexpr unsigned int PAD_VF = 3;\n> +\tstatic constexpr unsigned int PAD_STAT = 4;\n> +\n>  \tImgUDevice()\n>  \t\t: imgu(nullptr), input(nullptr), output(nullptr),\n>  \t\t  viewfinder(nullptr), stat(nullptr)\n> @@ -135,6 +141,13 @@ private:\n>  \n>  \tint initImgU(ImgUDevice *imgu);\n>  \n> +\tint setImguFormat(ImgUDevice *imguDevice,\n> +\t\t\t  const StreamConfiguration &config,\n> +\t\t\t  Rectangle *rect);\n\nThis should be moved to the ImgU class.\n\n> +\tint setCIO2Format(CIO2Device *cio2Device,\n> +\t\t\t  const StreamConfiguration &config,\n> +\t\t\t  V4L2SubdeviceFormat *format);\n> +\n>  \tvoid registerCameras();\n>  \n>  \tImgUDevice imgus_[IPU3_IMGU_COUNT];\n> @@ -202,60 +215,86 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n>  \t\t\t\t\t  std::map<Stream *, StreamConfiguration> &config)\n>  {\n>  \tIPU3CameraData *data = cameraData(camera);\n> -\tStreamConfiguration *cfg = &config[&data->stream_];\n> -\tV4L2Subdevice *sensor = data->cio2.sensor;\n> -\tV4L2Subdevice *csi2 = data->cio2.csi2;\n> +\tconst StreamConfiguration &cfg = config[&data->stream_];\n> +\tV4L2Device *viewfinder = data->imgu->viewfinder;\n> +\tV4L2Device *output = data->imgu->output;\n> +\tV4L2Device *input = data->imgu->input;\n>  \tV4L2Device *cio2 = data->cio2.output;\n> -\tV4L2SubdeviceFormat subdevFormat = {};\n> -\tV4L2DeviceFormat devFormat = {};\n>  \tint ret;\n>  \n> +\tLOG(IPU3, Info)\n> +\t\t<< \"Requested image format: \" << cfg.width << \"x\"\n> +\t\t<< cfg.height << \" - \" << std::hex << std::setw(8)\n> +\t\t<< cfg.pixelFormat << \" on camera:'\" << camera->name() << \"'\";\n> +\n>  \t/*\n> -\t * FIXME: as of now, the format gets applied to the sensor and is\n> -\t * propagated along the pipeline. It should instead be applied on the\n> -\t * capture device and the sensor format calculated accordingly.\n> +\t * Verify that the requested size respects the IPU3 alignement\n> +\t * requirements: the image width shall be a multiple of 8 pixels and\n> +\t * its height a multiple of 4 pixels.\n> +\t *\n> +\t * \\todo: consider the BDS scaling factor requirements:\n> +\t * \"the downscaling factor must be an integer value multiple of 1/32\"\n>  \t */\n> +\tif (cfg.width % 8 || cfg.height % 4) {\n> +\t\tLOG(IPU3, Error) << \"Stream format not support: bad alignement\";\n> +\t\treturn -EINVAL;\n> +\t}\n>  \n> -\tret = sensor->getFormat(0, &subdevFormat);\n> +\t/*\n> +\t * Pass the requested output image size to the sensor and get back the\n> +\t * adjusted one to be propagated to the CIO2 device and to the ImgU\n> +\t * input.\n> +\t */\n> +\tV4L2SubdeviceFormat sensorFormat = {};\n> +\tret = setCIO2Format(&data->cio2, cfg, &sensorFormat);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tsubdevFormat.width = cfg->width;\n> -\tsubdevFormat.height = cfg->height;\n> -\tret = sensor->setFormat(0, &subdevFormat);\n> +\t/* Apply the CIO2 image format to the CIO2 output and ImgU input. */\n> +\tV4L2DeviceFormat cio2Format = {};\n> +\tcio2Format.width = sensorFormat.width;\n> +\tcio2Format.height = sensorFormat.height;\n> +\tcio2Format.fourcc = mediaBusToCIO2Format(sensorFormat.mbus_code);\n> +\tcio2Format.planesCount = 1;\n> +\tret = cio2->setFormat(&cio2Format);\n\nShouldn't this be part of setCIO2Format() ?\n\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\t/* Return error if the requested format cannot be applied to sensor. */\n> -\tif (subdevFormat.width != cfg->width ||\n> -\t    subdevFormat.height != cfg->height) {\n> -\t\tLOG(IPU3, Error)\n> -\t\t\t<< \"Failed to apply image format \"\n> -\t\t\t<< subdevFormat.width << \"x\" << subdevFormat.height\n> -\t\t\t<< \" - got: \" << cfg->width << \"x\" << cfg->height;\n> -\t\treturn -EINVAL;\n> -\t}\n> +\tLOG(IPU3, Debug)\n> +\t\t<< \"CIO2 output format = \" << cio2Format.toString();\n>  \n> -\tret = csi2->setFormat(0, &subdevFormat);\n> +\tret = input->setFormat(&cio2Format);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tret = cio2->getFormat(&devFormat);\n> +\t/* Apply pad formats and crop/compose rectangle to the ImgU. */\n> +\tRectangle rect = {\n> +\t\t.x = 0,\n> +\t\t.y = 0,\n> +\t\t.w = cio2Format.width,\n> +\t\t.h = cio2Format.height,\n> +\t};\n> +\tret = setImguFormat(data->imgu, cfg, &rect);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tdevFormat.width = subdevFormat.width;\n> -\tdevFormat.height = subdevFormat.height;\n> -\tdevFormat.fourcc = cfg->pixelFormat;\n> +\t/* Apply the format to the ImgU output and viewfinder devices. */\n> +\tV4L2DeviceFormat outputFormat = {};\n> +\toutputFormat.width = cfg.width;\n> +\toutputFormat.height = cfg.height;\n> +\toutputFormat.fourcc = V4L2_PIX_FMT_NV12;\n> +\toutputFormat.planesCount = 2;\n>  \n> -\tret = cio2->setFormat(&devFormat);\n> +\tret = output->setFormat(&outputFormat);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tLOG(IPU3, Info) << cio2->driverName() << \": \"\n> -\t\t\t<< devFormat.width << \"x\" << devFormat.height\n> -\t\t\t<< \"- 0x\" << std::hex << devFormat.fourcc << \" planes: \"\n> -\t\t\t<< devFormat.planes;\n> +\tLOG(IPU3, Debug)\n> +\t\t<< \"ImgU output format = \" << outputFormat.toString();\n> +\n> +\tret = viewfinder->setFormat(&outputFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n>  \n>  \treturn 0;\n>  }\n> @@ -666,6 +705,125 @@ void PipelineHandlerIPU3::deleteCIO2(CIO2Device *cio2)\n>  \tdelete cio2->sensor;\n>  }\n>  \n> +int PipelineHandlerIPU3::setImguFormat(ImgUDevice *imguDevice,\n> +\t\t\t\t       const StreamConfiguration &config,\n> +\t\t\t\t       Rectangle *rect)\n> +{\n> +\tV4L2Subdevice *imgu = imguDevice->imgu;\n> +\tint ret;\n> +\n> +\t/*\n> +\t * Configure the 'imgu' subdevice with the requested sizes.\n\ns/'imgu'/ImgU/ ?\n\n> +\t *\n> +\t * FIXME: the IPU3 driver implementation shall be changed to use the\n> +\t * actual input sizes as 'imgu input' subdevice sizes, and use the\n> +\t * desired output sizes to configure the crop/compose rectangles.  The\n> +\t * current implementation uses output sizes as 'imgu input' sizes, and\n> +\t * uses the input dimension to configure the crop/compose rectangles,\n> +\t * which contradicts the V4L2 specifications.\n\ns/specifications/specification/\n\n> +\t */\n> +\tret = imgu->setCrop(ImgUDevice::PAD_INPUT, rect);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = imgu->setCompose(ImgUDevice::PAD_INPUT, rect);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tLOG(IPU3, Debug)\n> +\t\t<< \"ImgU input feeder and BDS rectangle = (0,0)/\"\n> +\t\t<< rect->w << \"x\" << rect->h;\n\ntoString() for our Rectangle class ?\n\n> +\n> +\tV4L2SubdeviceFormat imguFormat = {};\n> +\timguFormat.width = config.width;\n> +\timguFormat.height = config.height;\n> +\timguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;\n> +\n> +\tret = imgu->setFormat(ImgUDevice::PAD_INPUT, &imguFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = imgu->setFormat(ImgUDevice::PAD_OUTPUT, &imguFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tLOG(IPU3, Debug)\n> +\t\t<< \"ImgU GDC format = \" << imguFormat.toString();\n> +\n> +\tret = imgu->setFormat(ImgUDevice::PAD_VF, &imguFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = imgu->setFormat(ImgUDevice::PAD_STAT, &imguFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int PipelineHandlerIPU3::setCIO2Format(CIO2Device *cio2Device,\n> +\t\t\t\t       const StreamConfiguration &config,\n> +\t\t\t\t       V4L2SubdeviceFormat *format)\n> +{\n\nWe're starting to get quite some code associated with the CIO2Device. I\nknow I requested for CIO2Device to be a struct without methods, but I\nwonder if that was a mistake :-(\n\nI would also rename the function, maybe to configure(). setFormat()\nhints that it sets the format passed as an argument, which is not the\ncase here, the format parameter is an output.\n\n> +\tunsigned int imageSize = config.width * config.height;\n> +\tV4L2Subdevice *sensor = cio2Device->sensor;\n> +\tV4L2Subdevice *csi2 = cio2Device->csi2;\n> +\tunsigned int best = ~0;\n> +\tbool found = false;\n> +\tint ret;\n> +\n> +\tconst FormatEnum formats = sensor->formats(0);\n> +\tfor (auto it = formats.begin(); it != formats.end(); ++it) {\n> +\t\t/* Only consider formats consumable by the CIO2 unit. */\n> +\t\tint cio2Code = mediaBusToCIO2Format(it->first);\n> +\t\tif (cio2Code == -EINVAL)\n\nYou could write\n\n\t\tif (mediaBusToCIO2Format(it->first) == -EINVAL)\n\n(and I would check for < 0 instead of == -EINVAL to avoid surprises if\nwe later change that method)\n\n> +\t\t\tcontinue;\n> +\n> +\t\tfor (const SizeRange &size : it->second) {\n> +\t\t\t/*\n> +\t\t\t * Only select formats bigger than the requested sizes\n> +\t\t\t * as the IPU3 cannot up-scale.\n> +\t\t\t */\n> +\t\t\tif (size.maxWidth < config.width ||\n> +\t\t\t    size.maxHeight < config.height)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tunsigned int diff = size.maxWidth * size.maxHeight\n> +\t\t\t\t\t  - imageSize;\n> +\t\t\tif (diff >= best)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tbest = diff;\n> +\t\t\tfound = true;\n> +\n> +\t\t\tformat->width = size.maxWidth;\n> +\t\t\tformat->height = size.maxHeight;\n> +\t\t\tformat->mbus_code = it->first;\n> +\t\t}\n> +\t}\n\nOpen question, should we do this, or always pick the largest size from\nthe sensor and scale in the ImgU ?\n\n> +\tif (!found) {\n> +\t\tLOG(IPU3, Error)\n> +\t\t\t<< \"Unable to find image format suitable to produce: \"\n> +\t\t\t<< config.width << \"x\" << config.height\n> +\t\t\t<< \"- 0x\" << std::hex << std::setfill('0')\n\nSpaces before and after dash, or none at all.\n\n> +\t\t\t<< std::setw(8) << config.pixelFormat;\n\nAgain a shame we can't use toString(). Do you use that helper at all ?\n:-)\n\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\t/* Apply the selected format to the sensor and the CSI-2 receiver. */\n> +\tret = sensor->setFormat(0, format);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = csi2->setFormat(0, format);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tLOG(IPU3, Debug) << \"CIO2 Image format: \" << format->toString();\n\nAh yes you use it here.\n\n> +\n> +\treturn 0;\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":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 21E45600F9\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 21 Mar 2019 10:54:31 +0100 (CET)","from pendragon.ideasonboard.com (30.net042126252.t-com.ne.jp\n\t[42.126.252.30])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7373B23A;\n\tThu, 21 Mar 2019 10:54:29 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1553162070;\n\tbh=1vvMWKL7KKj3CgkTxVZTi51yp/nQ7/N8/aYIbUXRgNg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kmoWc0YHFt2JVAzjzodkc0iHJxnsxJqlP1mP85B+O0nrVWpAgB3ta6njD804T+Dbt\n\tAP4rpEdKSIJohI0CYVeQ+DqioZrz7Ny9571UdleSF5nRRzHNF3H6FeRAC9ag12EOOc\n\tDrsY32yBuPxMXPil4/TZxCJjnGwixumFnNIu9XnQ=","Date":"Thu, 21 Mar 2019 11:54:18 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190321095418.GJ4615@pendragon.ideasonboard.com>","References":"<20190320163055.22056-1-jacopo@jmondi.org>\n\t<20190320163055.22056-8-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190320163055.22056-8-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 07/31] libcamera: ipu3: Propagate\n\timage format","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Thu, 21 Mar 2019 09:54:31 -0000"}},{"id":1096,"web_url":"https://patchwork.libcamera.org/comment/1096/","msgid":"<20190321193112.oxxel6mdvkcot3yq@uno.localdomain>","date":"2019-03-21T19:31:12","subject":"Re: [libcamera-devel] [PATCH v4 07/31] libcamera: ipu3: Propagate\n\timage format","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Thu, Mar 21, 2019 at 11:54:18AM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Mar 20, 2019 at 05:30:31PM +0100, Jacopo Mondi wrote:\n> > Apply the requested image format to the sensor device, and apply the\n> > adjusted one to the CIO2 device, the ImgU subdevice and its input and\n> > output video devices.\n> >\n> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > ---\n> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 220 +++++++++++++++++++++++----\n> >  1 file changed, 189 insertions(+), 31 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 26fc56a76eb1..2975c218f6c9 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -16,6 +16,7 @@\n> >  #include <libcamera/stream.h>\n> >\n> >  #include \"device_enumerator.h\"\n> > +#include \"geometry.h\"\n> >  #include \"log.h\"\n> >  #include \"media_device.h\"\n> >  #include \"pipeline_handler.h\"\n> > @@ -30,6 +31,11 @@ LOG_DEFINE_CATEGORY(IPU3)\n> >  class ImgUDevice\n> >  {\n> >  public:\n> > +\tstatic constexpr unsigned int PAD_INPUT = 0;\n> > +\tstatic constexpr unsigned int PAD_OUTPUT = 2;\n> > +\tstatic constexpr unsigned int PAD_VF = 3;\n> > +\tstatic constexpr unsigned int PAD_STAT = 4;\n> > +\n> >  \tImgUDevice()\n> >  \t\t: imgu(nullptr), input(nullptr), output(nullptr),\n> >  \t\t  viewfinder(nullptr), stat(nullptr)\n> > @@ -135,6 +141,13 @@ private:\n> >\n> >  \tint initImgU(ImgUDevice *imgu);\n> >\n> > +\tint setImguFormat(ImgUDevice *imguDevice,\n> > +\t\t\t  const StreamConfiguration &config,\n> > +\t\t\t  Rectangle *rect);\n>\n> This should be moved to the ImgU class.\n>\n> > +\tint setCIO2Format(CIO2Device *cio2Device,\n> > +\t\t\t  const StreamConfiguration &config,\n> > +\t\t\t  V4L2SubdeviceFormat *format);\n\nand this to the CIO2 class too?\n\n\n> > +\n> >  \tvoid registerCameras();\n> >\n> >  \tImgUDevice imgus_[IPU3_IMGU_COUNT];\n> > @@ -202,60 +215,86 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> >  \t\t\t\t\t  std::map<Stream *, StreamConfiguration> &config)\n> >  {\n> >  \tIPU3CameraData *data = cameraData(camera);\n> > -\tStreamConfiguration *cfg = &config[&data->stream_];\n> > -\tV4L2Subdevice *sensor = data->cio2.sensor;\n> > -\tV4L2Subdevice *csi2 = data->cio2.csi2;\n> > +\tconst StreamConfiguration &cfg = config[&data->stream_];\n> > +\tV4L2Device *viewfinder = data->imgu->viewfinder;\n> > +\tV4L2Device *output = data->imgu->output;\n> > +\tV4L2Device *input = data->imgu->input;\n> >  \tV4L2Device *cio2 = data->cio2.output;\n> > -\tV4L2SubdeviceFormat subdevFormat = {};\n> > -\tV4L2DeviceFormat devFormat = {};\n> >  \tint ret;\n> >\n> > +\tLOG(IPU3, Info)\n> > +\t\t<< \"Requested image format: \" << cfg.width << \"x\"\n> > +\t\t<< cfg.height << \" - \" << std::hex << std::setw(8)\n> > +\t\t<< cfg.pixelFormat << \" on camera:'\" << camera->name() << \"'\";\n> > +\n> >  \t/*\n> > -\t * FIXME: as of now, the format gets applied to the sensor and is\n> > -\t * propagated along the pipeline. It should instead be applied on the\n> > -\t * capture device and the sensor format calculated accordingly.\n> > +\t * Verify that the requested size respects the IPU3 alignement\n> > +\t * requirements: the image width shall be a multiple of 8 pixels and\n> > +\t * its height a multiple of 4 pixels.\n> > +\t *\n> > +\t * \\todo: consider the BDS scaling factor requirements:\n> > +\t * \"the downscaling factor must be an integer value multiple of 1/32\"\n> >  \t */\n> > +\tif (cfg.width % 8 || cfg.height % 4) {\n> > +\t\tLOG(IPU3, Error) << \"Stream format not support: bad alignement\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> >\n> > -\tret = sensor->getFormat(0, &subdevFormat);\n> > +\t/*\n> > +\t * Pass the requested output image size to the sensor and get back the\n> > +\t * adjusted one to be propagated to the CIO2 device and to the ImgU\n> > +\t * input.\n> > +\t */\n> > +\tV4L2SubdeviceFormat sensorFormat = {};\n> > +\tret = setCIO2Format(&data->cio2, cfg, &sensorFormat);\n> >  \tif (ret)\n> >  \t\treturn ret;\n> >\n> > -\tsubdevFormat.width = cfg->width;\n> > -\tsubdevFormat.height = cfg->height;\n> > -\tret = sensor->setFormat(0, &subdevFormat);\n> > +\t/* Apply the CIO2 image format to the CIO2 output and ImgU input. */\n> > +\tV4L2DeviceFormat cio2Format = {};\n> > +\tcio2Format.width = sensorFormat.width;\n> > +\tcio2Format.height = sensorFormat.height;\n> > +\tcio2Format.fourcc = mediaBusToCIO2Format(sensorFormat.mbus_code);\n> > +\tcio2Format.planesCount = 1;\n> > +\tret = cio2->setFormat(&cio2Format);\n>\n> Shouldn't this be part of setCIO2Format() ?\n\nMoved there. I wanted to expose the fact the CIO2 output and the ImgU\ninput are configured with the same format.\n>\n> >  \tif (ret)\n> >  \t\treturn ret;\n> >\n> > -\t/* Return error if the requested format cannot be applied to sensor. */\n> > -\tif (subdevFormat.width != cfg->width ||\n> > -\t    subdevFormat.height != cfg->height) {\n> > -\t\tLOG(IPU3, Error)\n> > -\t\t\t<< \"Failed to apply image format \"\n> > -\t\t\t<< subdevFormat.width << \"x\" << subdevFormat.height\n> > -\t\t\t<< \" - got: \" << cfg->width << \"x\" << cfg->height;\n> > -\t\treturn -EINVAL;\n> > -\t}\n> > +\tLOG(IPU3, Debug)\n> > +\t\t<< \"CIO2 output format = \" << cio2Format.toString();\n> >\n> > -\tret = csi2->setFormat(0, &subdevFormat);\n> > +\tret = input->setFormat(&cio2Format);\n> >  \tif (ret)\n> >  \t\treturn ret;\n> >\n> > -\tret = cio2->getFormat(&devFormat);\n> > +\t/* Apply pad formats and crop/compose rectangle to the ImgU. */\n> > +\tRectangle rect = {\n> > +\t\t.x = 0,\n> > +\t\t.y = 0,\n> > +\t\t.w = cio2Format.width,\n> > +\t\t.h = cio2Format.height,\n> > +\t};\n> > +\tret = setImguFormat(data->imgu, cfg, &rect);\n> >  \tif (ret)\n> >  \t\treturn ret;\n> >\n> > -\tdevFormat.width = subdevFormat.width;\n> > -\tdevFormat.height = subdevFormat.height;\n> > -\tdevFormat.fourcc = cfg->pixelFormat;\n> > +\t/* Apply the format to the ImgU output and viewfinder devices. */\n> > +\tV4L2DeviceFormat outputFormat = {};\n> > +\toutputFormat.width = cfg.width;\n> > +\toutputFormat.height = cfg.height;\n> > +\toutputFormat.fourcc = V4L2_PIX_FMT_NV12;\n> > +\toutputFormat.planesCount = 2;\n> >\n> > -\tret = cio2->setFormat(&devFormat);\n> > +\tret = output->setFormat(&outputFormat);\n> >  \tif (ret)\n> >  \t\treturn ret;\n> >\n> > -\tLOG(IPU3, Info) << cio2->driverName() << \": \"\n> > -\t\t\t<< devFormat.width << \"x\" << devFormat.height\n> > -\t\t\t<< \"- 0x\" << std::hex << devFormat.fourcc << \" planes: \"\n> > -\t\t\t<< devFormat.planes;\n> > +\tLOG(IPU3, Debug)\n> > +\t\t<< \"ImgU output format = \" << outputFormat.toString();\n> > +\n> > +\tret = viewfinder->setFormat(&outputFormat);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> >\n> >  \treturn 0;\n> >  }\n> > @@ -666,6 +705,125 @@ void PipelineHandlerIPU3::deleteCIO2(CIO2Device *cio2)\n> >  \tdelete cio2->sensor;\n> >  }\n> >\n> > +int PipelineHandlerIPU3::setImguFormat(ImgUDevice *imguDevice,\n> > +\t\t\t\t       const StreamConfiguration &config,\n> > +\t\t\t\t       Rectangle *rect)\n> > +{\n> > +\tV4L2Subdevice *imgu = imguDevice->imgu;\n> > +\tint ret;\n> > +\n> > +\t/*\n> > +\t * Configure the 'imgu' subdevice with the requested sizes.\n>\n> s/'imgu'/ImgU/ ?\n>\n> > +\t *\n> > +\t * FIXME: the IPU3 driver implementation shall be changed to use the\n> > +\t * actual input sizes as 'imgu input' subdevice sizes, and use the\n> > +\t * desired output sizes to configure the crop/compose rectangles.  The\n> > +\t * current implementation uses output sizes as 'imgu input' sizes, and\n> > +\t * uses the input dimension to configure the crop/compose rectangles,\n> > +\t * which contradicts the V4L2 specifications.\n>\n> s/specifications/specification/\n>\n> > +\t */\n> > +\tret = imgu->setCrop(ImgUDevice::PAD_INPUT, rect);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tret = imgu->setCompose(ImgUDevice::PAD_INPUT, rect);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tLOG(IPU3, Debug)\n> > +\t\t<< \"ImgU input feeder and BDS rectangle = (0,0)/\"\n> > +\t\t<< rect->w << \"x\" << rect->h;\n>\n> toString() for our Rectangle class ?\n>\n\nI'll add a patch\n\n> > +\n> > +\tV4L2SubdeviceFormat imguFormat = {};\n> > +\timguFormat.width = config.width;\n> > +\timguFormat.height = config.height;\n> > +\timguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;\n> > +\n> > +\tret = imgu->setFormat(ImgUDevice::PAD_INPUT, &imguFormat);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tret = imgu->setFormat(ImgUDevice::PAD_OUTPUT, &imguFormat);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tLOG(IPU3, Debug)\n> > +\t\t<< \"ImgU GDC format = \" << imguFormat.toString();\n> > +\n> > +\tret = imgu->setFormat(ImgUDevice::PAD_VF, &imguFormat);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tret = imgu->setFormat(ImgUDevice::PAD_STAT, &imguFormat);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int PipelineHandlerIPU3::setCIO2Format(CIO2Device *cio2Device,\n> > +\t\t\t\t       const StreamConfiguration &config,\n> > +\t\t\t\t       V4L2SubdeviceFormat *format)\n> > +{\n>\n> We're starting to get quite some code associated with the CIO2Device. I\n> know I requested for CIO2Device to be a struct without methods, but I\n> wonder if that was a mistake :-(\n>\n> I would also rename the function, maybe to configure(). setFormat()\n> hints that it sets the format passed as an argument, which is not the\n> case here, the format parameter is an output.\n>\n> > +\tunsigned int imageSize = config.width * config.height;\n> > +\tV4L2Subdevice *sensor = cio2Device->sensor;\n> > +\tV4L2Subdevice *csi2 = cio2Device->csi2;\n> > +\tunsigned int best = ~0;\n> > +\tbool found = false;\n> > +\tint ret;\n> > +\n> > +\tconst FormatEnum formats = sensor->formats(0);\n> > +\tfor (auto it = formats.begin(); it != formats.end(); ++it) {\n> > +\t\t/* Only consider formats consumable by the CIO2 unit. */\n> > +\t\tint cio2Code = mediaBusToCIO2Format(it->first);\n> > +\t\tif (cio2Code == -EINVAL)\n>\n> You could write\n>\n> \t\tif (mediaBusToCIO2Format(it->first) == -EINVAL)\n>\n> (and I would check for < 0 instead of == -EINVAL to avoid surprises if\n> we later change that method)\n>\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tfor (const SizeRange &size : it->second) {\n> > +\t\t\t/*\n> > +\t\t\t * Only select formats bigger than the requested sizes\n> > +\t\t\t * as the IPU3 cannot up-scale.\n> > +\t\t\t */\n> > +\t\t\tif (size.maxWidth < config.width ||\n> > +\t\t\t    size.maxHeight < config.height)\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\tunsigned int diff = size.maxWidth * size.maxHeight\n> > +\t\t\t\t\t  - imageSize;\n> > +\t\t\tif (diff >= best)\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\tbest = diff;\n> > +\t\t\tfound = true;\n> > +\n> > +\t\t\tformat->width = size.maxWidth;\n> > +\t\t\tformat->height = size.maxHeight;\n> > +\t\t\tformat->mbus_code = it->first;\n> > +\t\t}\n> > +\t}\n>\n> Open question, should we do this, or always pick the largest size from\n> the sensor and scale in the ImgU ?\n>\n\nGood question... I cannot tell the pros and cons actually... why would\nyou think it is better?\n\nThanks\n  j\n\n> > +\tif (!found) {\n> > +\t\tLOG(IPU3, Error)\n> > +\t\t\t<< \"Unable to find image format suitable to produce: \"\n> > +\t\t\t<< config.width << \"x\" << config.height\n> > +\t\t\t<< \"- 0x\" << std::hex << std::setfill('0')\n>\n> Spaces before and after dash, or none at all.\n>\n> > +\t\t\t<< std::setw(8) << config.pixelFormat;\n>\n> Again a shame we can't use toString(). Do you use that helper at all ?\n> :-)\n>\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\t/* Apply the selected format to the sensor and the CSI-2 receiver. */\n> > +\tret = sensor->setFormat(0, format);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tret = csi2->setFormat(0, format);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tLOG(IPU3, Debug) << \"CIO2 Image format: \" << format->toString();\n>\n> Ah yes you use it here.\n>\n> > +\n> > +\treturn 0;\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":"<jacopo@jmondi.org>","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 EE257600FD\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 21 Mar 2019 20:30:33 +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 57F22E0002;\n\tThu, 21 Mar 2019 19:30:33 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 21 Mar 2019 20:31:12 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190321193112.oxxel6mdvkcot3yq@uno.localdomain>","References":"<20190320163055.22056-1-jacopo@jmondi.org>\n\t<20190320163055.22056-8-jacopo@jmondi.org>\n\t<20190321095418.GJ4615@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"7durufzefwg7lym2\"","Content-Disposition":"inline","In-Reply-To":"<20190321095418.GJ4615@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v4 07/31] libcamera: ipu3: Propagate\n\timage format","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Thu, 21 Mar 2019 19:30:34 -0000"}},{"id":1103,"web_url":"https://patchwork.libcamera.org/comment/1103/","msgid":"<20190323121746.dvfgrobfzm42tuse@uno.localdomain>","date":"2019-03-23T12:17:46","subject":"Re: [libcamera-devel] [PATCH v4 07/31] libcamera: ipu3: Propagate\n\timage format","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Thu, Mar 21, 2019 at 08:31:12PM +0100, Jacopo Mondi wrote:\n> Hi Laurent,\n>\n> On Thu, Mar 21, 2019 at 11:54:18AM +0200, Laurent Pinchart wrote:\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n> >\n> > On Wed, Mar 20, 2019 at 05:30:31PM +0100, Jacopo Mondi wrote:\n> > > Apply the requested image format to the sensor device, and apply the\n> > > adjusted one to the CIO2 device, the ImgU subdevice and its input and\n> > > output video devices.\n> > >\n> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > > ---\n> > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 220 +++++++++++++++++++++++----\n> > >  1 file changed, 189 insertions(+), 31 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 26fc56a76eb1..2975c218f6c9 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -16,6 +16,7 @@\n> > >  #include <libcamera/stream.h>\n> > >\n> > >  #include \"device_enumerator.h\"\n> > > +#include \"geometry.h\"\n> > >  #include \"log.h\"\n> > >  #include \"media_device.h\"\n> > >  #include \"pipeline_handler.h\"\n> > > @@ -30,6 +31,11 @@ LOG_DEFINE_CATEGORY(IPU3)\n> > >  class ImgUDevice\n> > >  {\n> > >  public:\n> > > +\tstatic constexpr unsigned int PAD_INPUT = 0;\n> > > +\tstatic constexpr unsigned int PAD_OUTPUT = 2;\n> > > +\tstatic constexpr unsigned int PAD_VF = 3;\n> > > +\tstatic constexpr unsigned int PAD_STAT = 4;\n> > > +\n> > >  \tImgUDevice()\n> > >  \t\t: imgu(nullptr), input(nullptr), output(nullptr),\n> > >  \t\t  viewfinder(nullptr), stat(nullptr)\n> > > @@ -135,6 +141,13 @@ private:\n> > >\n> > >  \tint initImgU(ImgUDevice *imgu);\n> > >\n> > > +\tint setImguFormat(ImgUDevice *imguDevice,\n> > > +\t\t\t  const StreamConfiguration &config,\n> > > +\t\t\t  Rectangle *rect);\n> >\n> > This should be moved to the ImgU class.\n> >\n> > > +\tint setCIO2Format(CIO2Device *cio2Device,\n> > > +\t\t\t  const StreamConfiguration &config,\n> > > +\t\t\t  V4L2SubdeviceFormat *format);\n>\n> and this to the CIO2 class too?\n>\n>\n> > > +\n> > >  \tvoid registerCameras();\n> > >\n> > >  \tImgUDevice imgus_[IPU3_IMGU_COUNT];\n> > > @@ -202,60 +215,86 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> > >  \t\t\t\t\t  std::map<Stream *, StreamConfiguration> &config)\n> > >  {\n> > >  \tIPU3CameraData *data = cameraData(camera);\n> > > -\tStreamConfiguration *cfg = &config[&data->stream_];\n> > > -\tV4L2Subdevice *sensor = data->cio2.sensor;\n> > > -\tV4L2Subdevice *csi2 = data->cio2.csi2;\n> > > +\tconst StreamConfiguration &cfg = config[&data->stream_];\n> > > +\tV4L2Device *viewfinder = data->imgu->viewfinder;\n> > > +\tV4L2Device *output = data->imgu->output;\n> > > +\tV4L2Device *input = data->imgu->input;\n> > >  \tV4L2Device *cio2 = data->cio2.output;\n> > > -\tV4L2SubdeviceFormat subdevFormat = {};\n> > > -\tV4L2DeviceFormat devFormat = {};\n> > >  \tint ret;\n> > >\n> > > +\tLOG(IPU3, Info)\n> > > +\t\t<< \"Requested image format: \" << cfg.width << \"x\"\n> > > +\t\t<< cfg.height << \" - \" << std::hex << std::setw(8)\n> > > +\t\t<< cfg.pixelFormat << \" on camera:'\" << camera->name() << \"'\";\n> > > +\n> > >  \t/*\n> > > -\t * FIXME: as of now, the format gets applied to the sensor and is\n> > > -\t * propagated along the pipeline. It should instead be applied on the\n> > > -\t * capture device and the sensor format calculated accordingly.\n> > > +\t * Verify that the requested size respects the IPU3 alignement\n> > > +\t * requirements: the image width shall be a multiple of 8 pixels and\n> > > +\t * its height a multiple of 4 pixels.\n> > > +\t *\n> > > +\t * \\todo: consider the BDS scaling factor requirements:\n> > > +\t * \"the downscaling factor must be an integer value multiple of 1/32\"\n> > >  \t */\n> > > +\tif (cfg.width % 8 || cfg.height % 4) {\n> > > +\t\tLOG(IPU3, Error) << \"Stream format not support: bad alignement\";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > >\n> > > -\tret = sensor->getFormat(0, &subdevFormat);\n> > > +\t/*\n> > > +\t * Pass the requested output image size to the sensor and get back the\n> > > +\t * adjusted one to be propagated to the CIO2 device and to the ImgU\n> > > +\t * input.\n> > > +\t */\n> > > +\tV4L2SubdeviceFormat sensorFormat = {};\n> > > +\tret = setCIO2Format(&data->cio2, cfg, &sensorFormat);\n> > >  \tif (ret)\n> > >  \t\treturn ret;\n> > >\n> > > -\tsubdevFormat.width = cfg->width;\n> > > -\tsubdevFormat.height = cfg->height;\n> > > -\tret = sensor->setFormat(0, &subdevFormat);\n> > > +\t/* Apply the CIO2 image format to the CIO2 output and ImgU input. */\n> > > +\tV4L2DeviceFormat cio2Format = {};\n> > > +\tcio2Format.width = sensorFormat.width;\n> > > +\tcio2Format.height = sensorFormat.height;\n> > > +\tcio2Format.fourcc = mediaBusToCIO2Format(sensorFormat.mbus_code);\n> > > +\tcio2Format.planesCount = 1;\n> > > +\tret = cio2->setFormat(&cio2Format);\n> >\n> > Shouldn't this be part of setCIO2Format() ?\n>\n> Moved there. I wanted to expose the fact the CIO2 output and the ImgU\n> input are configured with the same format.\n> >\n> > >  \tif (ret)\n> > >  \t\treturn ret;\n> > >\n> > > -\t/* Return error if the requested format cannot be applied to sensor. */\n> > > -\tif (subdevFormat.width != cfg->width ||\n> > > -\t    subdevFormat.height != cfg->height) {\n> > > -\t\tLOG(IPU3, Error)\n> > > -\t\t\t<< \"Failed to apply image format \"\n> > > -\t\t\t<< subdevFormat.width << \"x\" << subdevFormat.height\n> > > -\t\t\t<< \" - got: \" << cfg->width << \"x\" << cfg->height;\n> > > -\t\treturn -EINVAL;\n> > > -\t}\n> > > +\tLOG(IPU3, Debug)\n> > > +\t\t<< \"CIO2 output format = \" << cio2Format.toString();\n> > >\n> > > -\tret = csi2->setFormat(0, &subdevFormat);\n> > > +\tret = input->setFormat(&cio2Format);\n> > >  \tif (ret)\n> > >  \t\treturn ret;\n> > >\n> > > -\tret = cio2->getFormat(&devFormat);\n> > > +\t/* Apply pad formats and crop/compose rectangle to the ImgU. */\n> > > +\tRectangle rect = {\n> > > +\t\t.x = 0,\n> > > +\t\t.y = 0,\n> > > +\t\t.w = cio2Format.width,\n> > > +\t\t.h = cio2Format.height,\n> > > +\t};\n> > > +\tret = setImguFormat(data->imgu, cfg, &rect);\n> > >  \tif (ret)\n> > >  \t\treturn ret;\n> > >\n> > > -\tdevFormat.width = subdevFormat.width;\n> > > -\tdevFormat.height = subdevFormat.height;\n> > > -\tdevFormat.fourcc = cfg->pixelFormat;\n> > > +\t/* Apply the format to the ImgU output and viewfinder devices. */\n> > > +\tV4L2DeviceFormat outputFormat = {};\n> > > +\toutputFormat.width = cfg.width;\n> > > +\toutputFormat.height = cfg.height;\n> > > +\toutputFormat.fourcc = V4L2_PIX_FMT_NV12;\n> > > +\toutputFormat.planesCount = 2;\n> > >\n> > > -\tret = cio2->setFormat(&devFormat);\n> > > +\tret = output->setFormat(&outputFormat);\n> > >  \tif (ret)\n> > >  \t\treturn ret;\n> > >\n> > > -\tLOG(IPU3, Info) << cio2->driverName() << \": \"\n> > > -\t\t\t<< devFormat.width << \"x\" << devFormat.height\n> > > -\t\t\t<< \"- 0x\" << std::hex << devFormat.fourcc << \" planes: \"\n> > > -\t\t\t<< devFormat.planes;\n> > > +\tLOG(IPU3, Debug)\n> > > +\t\t<< \"ImgU output format = \" << outputFormat.toString();\n> > > +\n> > > +\tret = viewfinder->setFormat(&outputFormat);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > >\n> > >  \treturn 0;\n> > >  }\n> > > @@ -666,6 +705,125 @@ void PipelineHandlerIPU3::deleteCIO2(CIO2Device *cio2)\n> > >  \tdelete cio2->sensor;\n> > >  }\n> > >\n> > > +int PipelineHandlerIPU3::setImguFormat(ImgUDevice *imguDevice,\n> > > +\t\t\t\t       const StreamConfiguration &config,\n> > > +\t\t\t\t       Rectangle *rect)\n> > > +{\n> > > +\tV4L2Subdevice *imgu = imguDevice->imgu;\n> > > +\tint ret;\n> > > +\n> > > +\t/*\n> > > +\t * Configure the 'imgu' subdevice with the requested sizes.\n> >\n> > s/'imgu'/ImgU/ ?\n> >\n> > > +\t *\n> > > +\t * FIXME: the IPU3 driver implementation shall be changed to use the\n> > > +\t * actual input sizes as 'imgu input' subdevice sizes, and use the\n> > > +\t * desired output sizes to configure the crop/compose rectangles.  The\n> > > +\t * current implementation uses output sizes as 'imgu input' sizes, and\n> > > +\t * uses the input dimension to configure the crop/compose rectangles,\n> > > +\t * which contradicts the V4L2 specifications.\n> >\n> > s/specifications/specification/\n> >\n> > > +\t */\n> > > +\tret = imgu->setCrop(ImgUDevice::PAD_INPUT, rect);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tret = imgu->setCompose(ImgUDevice::PAD_INPUT, rect);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tLOG(IPU3, Debug)\n> > > +\t\t<< \"ImgU input feeder and BDS rectangle = (0,0)/\"\n> > > +\t\t<< rect->w << \"x\" << rect->h;\n> >\n> > toString() for our Rectangle class ?\n> >\n>\n> I'll add a patch\n>\n> > > +\n> > > +\tV4L2SubdeviceFormat imguFormat = {};\n> > > +\timguFormat.width = config.width;\n> > > +\timguFormat.height = config.height;\n> > > +\timguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;\n> > > +\n> > > +\tret = imgu->setFormat(ImgUDevice::PAD_INPUT, &imguFormat);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tret = imgu->setFormat(ImgUDevice::PAD_OUTPUT, &imguFormat);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tLOG(IPU3, Debug)\n> > > +\t\t<< \"ImgU GDC format = \" << imguFormat.toString();\n> > > +\n> > > +\tret = imgu->setFormat(ImgUDevice::PAD_VF, &imguFormat);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tret = imgu->setFormat(ImgUDevice::PAD_STAT, &imguFormat);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +int PipelineHandlerIPU3::setCIO2Format(CIO2Device *cio2Device,\n> > > +\t\t\t\t       const StreamConfiguration &config,\n> > > +\t\t\t\t       V4L2SubdeviceFormat *format)\n> > > +{\n> >\n> > We're starting to get quite some code associated with the CIO2Device. I\n> > know I requested for CIO2Device to be a struct without methods, but I\n> > wonder if that was a mistake :-(\n> >\n> > I would also rename the function, maybe to configure(). setFormat()\n> > hints that it sets the format passed as an argument, which is not the\n> > case here, the format parameter is an output.\n> >\n> > > +\tunsigned int imageSize = config.width * config.height;\n> > > +\tV4L2Subdevice *sensor = cio2Device->sensor;\n> > > +\tV4L2Subdevice *csi2 = cio2Device->csi2;\n> > > +\tunsigned int best = ~0;\n> > > +\tbool found = false;\n> > > +\tint ret;\n> > > +\n> > > +\tconst FormatEnum formats = sensor->formats(0);\n> > > +\tfor (auto it = formats.begin(); it != formats.end(); ++it) {\n> > > +\t\t/* Only consider formats consumable by the CIO2 unit. */\n> > > +\t\tint cio2Code = mediaBusToCIO2Format(it->first);\n> > > +\t\tif (cio2Code == -EINVAL)\n> >\n> > You could write\n> >\n> > \t\tif (mediaBusToCIO2Format(it->first) == -EINVAL)\n> >\n> > (and I would check for < 0 instead of == -EINVAL to avoid surprises if\n> > we later change that method)\n> >\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\tfor (const SizeRange &size : it->second) {\n> > > +\t\t\t/*\n> > > +\t\t\t * Only select formats bigger than the requested sizes\n> > > +\t\t\t * as the IPU3 cannot up-scale.\n> > > +\t\t\t */\n> > > +\t\t\tif (size.maxWidth < config.width ||\n> > > +\t\t\t    size.maxHeight < config.height)\n> > > +\t\t\t\tcontinue;\n> > > +\n> > > +\t\t\tunsigned int diff = size.maxWidth * size.maxHeight\n> > > +\t\t\t\t\t  - imageSize;\n> > > +\t\t\tif (diff >= best)\n> > > +\t\t\t\tcontinue;\n> > > +\n> > > +\t\t\tbest = diff;\n> > > +\t\t\tfound = true;\n> > > +\n> > > +\t\t\tformat->width = size.maxWidth;\n> > > +\t\t\tformat->height = size.maxHeight;\n> > > +\t\t\tformat->mbus_code = it->first;\n> > > +\t\t}\n> > > +\t}\n> >\n> > Open question, should we do this, or always pick the largest size from\n> > the sensor and scale in the ImgU ?\n> >\n>\n> Good question... I cannot tell the pros and cons actually... why would\n> you think it is better?\n>\n\nTo add a few elements, I looked into the xml files used to calculate\nthe input, BDS and GDC sizes at:\nhttps://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master/baseboard-poppy/media-libs/cros-camera-hal-configs-poppy/files/gcss/\n\nand it seems the sensor provided sizes are indeed selected matching\nthe output and viewfinder sizes.\n\nHow to calculate the GDB and BDS rectangle it's not clear at all at\nthe moment to me, I'll get back to the linux-media list to require for\nmore clarifications.\n\nThanks\n  j\n\n> Thanks\n>   j\n>\n> > > +\tif (!found) {\n> > > +\t\tLOG(IPU3, Error)\n> > > +\t\t\t<< \"Unable to find image format suitable to produce: \"\n> > > +\t\t\t<< config.width << \"x\" << config.height\n> > > +\t\t\t<< \"- 0x\" << std::hex << std::setfill('0')\n> >\n> > Spaces before and after dash, or none at all.\n> >\n> > > +\t\t\t<< std::setw(8) << config.pixelFormat;\n> >\n> > Again a shame we can't use toString(). Do you use that helper at all ?\n> > :-)\n> >\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\t/* Apply the selected format to the sensor and the CSI-2 receiver. */\n> > > +\tret = sensor->setFormat(0, format);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tret = csi2->setFormat(0, format);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tLOG(IPU3, Debug) << \"CIO2 Image format: \" << format->toString();\n> >\n> > Ah yes you use it here.\n> >\n> > > +\n> > > +\treturn 0;\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\n\n\n\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<jacopo@jmondi.org>","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 A618B610B6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 23 Mar 2019 13:17:06 +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 relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 0D55440003;\n\tSat, 23 Mar 2019 12:17:05 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Sat, 23 Mar 2019 13:17:46 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190323121746.dvfgrobfzm42tuse@uno.localdomain>","References":"<20190320163055.22056-1-jacopo@jmondi.org>\n\t<20190320163055.22056-8-jacopo@jmondi.org>\n\t<20190321095418.GJ4615@pendragon.ideasonboard.com>\n\t<20190321193112.oxxel6mdvkcot3yq@uno.localdomain>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"t6675dpul7rze5is\"","Content-Disposition":"inline","In-Reply-To":"<20190321193112.oxxel6mdvkcot3yq@uno.localdomain>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v4 07/31] libcamera: ipu3: Propagate\n\timage format","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Sat, 23 Mar 2019 12:17:06 -0000"}},{"id":1105,"web_url":"https://patchwork.libcamera.org/comment/1105/","msgid":"<20190323130034.GC4587@pendragon.ideasonboard.com>","date":"2019-03-23T13:00:34","subject":"Re: [libcamera-devel] [PATCH v4 07/31] libcamera: ipu3: Propagate\n\timage format","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Mar 21, 2019 at 08:31:12PM +0100, Jacopo Mondi wrote:\n> On Thu, Mar 21, 2019 at 11:54:18AM +0200, Laurent Pinchart wrote:\n> > On Wed, Mar 20, 2019 at 05:30:31PM +0100, Jacopo Mondi wrote:\n> >> Apply the requested image format to the sensor device, and apply the\n> >> adjusted one to the CIO2 device, the ImgU subdevice and its input and\n> >> output video devices.\n> >>\n> >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> >> ---\n> >>  src/libcamera/pipeline/ipu3/ipu3.cpp | 220 +++++++++++++++++++++++----\n> >>  1 file changed, 189 insertions(+), 31 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> index 26fc56a76eb1..2975c218f6c9 100644\n> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> @@ -16,6 +16,7 @@\n> >>  #include <libcamera/stream.h>\n> >>\n> >>  #include \"device_enumerator.h\"\n> >> +#include \"geometry.h\"\n> >>  #include \"log.h\"\n> >>  #include \"media_device.h\"\n> >>  #include \"pipeline_handler.h\"\n> >> @@ -30,6 +31,11 @@ LOG_DEFINE_CATEGORY(IPU3)\n> >>  class ImgUDevice\n> >>  {\n> >>  public:\n> >> +\tstatic constexpr unsigned int PAD_INPUT = 0;\n> >> +\tstatic constexpr unsigned int PAD_OUTPUT = 2;\n> >> +\tstatic constexpr unsigned int PAD_VF = 3;\n> >> +\tstatic constexpr unsigned int PAD_STAT = 4;\n> >> +\n> >>  \tImgUDevice()\n> >>  \t\t: imgu(nullptr), input(nullptr), output(nullptr),\n> >>  \t\t  viewfinder(nullptr), stat(nullptr)\n> >> @@ -135,6 +141,13 @@ private:\n> >>\n> >>  \tint initImgU(ImgUDevice *imgu);\n> >>\n> >> +\tint setImguFormat(ImgUDevice *imguDevice,\n> >> +\t\t\t  const StreamConfiguration &config,\n> >> +\t\t\t  Rectangle *rect);\n> >\n> > This should be moved to the ImgU class.\n> >\n> >> +\tint setCIO2Format(CIO2Device *cio2Device,\n> >> +\t\t\t  const StreamConfiguration &config,\n> >> +\t\t\t  V4L2SubdeviceFormat *format);\n> \n> and this to the CIO2 class too?\n> \n> >> +\n> >>  \tvoid registerCameras();\n> >>\n> >>  \tImgUDevice imgus_[IPU3_IMGU_COUNT];\n> >> @@ -202,60 +215,86 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> >>  \t\t\t\t\t  std::map<Stream *, StreamConfiguration> &config)\n> >>  {\n> >>  \tIPU3CameraData *data = cameraData(camera);\n> >> -\tStreamConfiguration *cfg = &config[&data->stream_];\n> >> -\tV4L2Subdevice *sensor = data->cio2.sensor;\n> >> -\tV4L2Subdevice *csi2 = data->cio2.csi2;\n> >> +\tconst StreamConfiguration &cfg = config[&data->stream_];\n> >> +\tV4L2Device *viewfinder = data->imgu->viewfinder;\n> >> +\tV4L2Device *output = data->imgu->output;\n> >> +\tV4L2Device *input = data->imgu->input;\n> >>  \tV4L2Device *cio2 = data->cio2.output;\n> >> -\tV4L2SubdeviceFormat subdevFormat = {};\n> >> -\tV4L2DeviceFormat devFormat = {};\n> >>  \tint ret;\n> >>\n> >> +\tLOG(IPU3, Info)\n> >> +\t\t<< \"Requested image format: \" << cfg.width << \"x\"\n> >> +\t\t<< cfg.height << \" - \" << std::hex << std::setw(8)\n> >> +\t\t<< cfg.pixelFormat << \" on camera:'\" << camera->name() << \"'\";\n> >> +\n> >>  \t/*\n> >> -\t * FIXME: as of now, the format gets applied to the sensor and is\n> >> -\t * propagated along the pipeline. It should instead be applied on the\n> >> -\t * capture device and the sensor format calculated accordingly.\n> >> +\t * Verify that the requested size respects the IPU3 alignement\n> >> +\t * requirements: the image width shall be a multiple of 8 pixels and\n> >> +\t * its height a multiple of 4 pixels.\n> >> +\t *\n> >> +\t * \\todo: consider the BDS scaling factor requirements:\n> >> +\t * \"the downscaling factor must be an integer value multiple of 1/32\"\n> >>  \t */\n> >> +\tif (cfg.width % 8 || cfg.height % 4) {\n> >> +\t\tLOG(IPU3, Error) << \"Stream format not support: bad alignement\";\n> >> +\t\treturn -EINVAL;\n> >> +\t}\n> >>\n> >> -\tret = sensor->getFormat(0, &subdevFormat);\n> >> +\t/*\n> >> +\t * Pass the requested output image size to the sensor and get back the\n> >> +\t * adjusted one to be propagated to the CIO2 device and to the ImgU\n> >> +\t * input.\n> >> +\t */\n> >> +\tV4L2SubdeviceFormat sensorFormat = {};\n> >> +\tret = setCIO2Format(&data->cio2, cfg, &sensorFormat);\n> >>  \tif (ret)\n> >>  \t\treturn ret;\n> >>\n> >> -\tsubdevFormat.width = cfg->width;\n> >> -\tsubdevFormat.height = cfg->height;\n> >> -\tret = sensor->setFormat(0, &subdevFormat);\n> >> +\t/* Apply the CIO2 image format to the CIO2 output and ImgU input. */\n> >> +\tV4L2DeviceFormat cio2Format = {};\n> >> +\tcio2Format.width = sensorFormat.width;\n> >> +\tcio2Format.height = sensorFormat.height;\n> >> +\tcio2Format.fourcc = mediaBusToCIO2Format(sensorFormat.mbus_code);\n> >> +\tcio2Format.planesCount = 1;\n> >> +\tret = cio2->setFormat(&cio2Format);\n> >\n> > Shouldn't this be part of setCIO2Format() ?\n> \n> Moved there. I wanted to expose the fact the CIO2 output and the ImgU\n> input are configured with the same format.\n> >\n> >>  \tif (ret)\n> >>  \t\treturn ret;\n> >>\n> >> -\t/* Return error if the requested format cannot be applied to sensor. */\n> >> -\tif (subdevFormat.width != cfg->width ||\n> >> -\t    subdevFormat.height != cfg->height) {\n> >> -\t\tLOG(IPU3, Error)\n> >> -\t\t\t<< \"Failed to apply image format \"\n> >> -\t\t\t<< subdevFormat.width << \"x\" << subdevFormat.height\n> >> -\t\t\t<< \" - got: \" << cfg->width << \"x\" << cfg->height;\n> >> -\t\treturn -EINVAL;\n> >> -\t}\n> >> +\tLOG(IPU3, Debug)\n> >> +\t\t<< \"CIO2 output format = \" << cio2Format.toString();\n> >>\n> >> -\tret = csi2->setFormat(0, &subdevFormat);\n> >> +\tret = input->setFormat(&cio2Format);\n> >>  \tif (ret)\n> >>  \t\treturn ret;\n> >>\n> >> -\tret = cio2->getFormat(&devFormat);\n> >> +\t/* Apply pad formats and crop/compose rectangle to the ImgU. */\n> >> +\tRectangle rect = {\n> >> +\t\t.x = 0,\n> >> +\t\t.y = 0,\n> >> +\t\t.w = cio2Format.width,\n> >> +\t\t.h = cio2Format.height,\n> >> +\t};\n> >> +\tret = setImguFormat(data->imgu, cfg, &rect);\n> >>  \tif (ret)\n> >>  \t\treturn ret;\n> >>\n> >> -\tdevFormat.width = subdevFormat.width;\n> >> -\tdevFormat.height = subdevFormat.height;\n> >> -\tdevFormat.fourcc = cfg->pixelFormat;\n> >> +\t/* Apply the format to the ImgU output and viewfinder devices. */\n> >> +\tV4L2DeviceFormat outputFormat = {};\n> >> +\toutputFormat.width = cfg.width;\n> >> +\toutputFormat.height = cfg.height;\n> >> +\toutputFormat.fourcc = V4L2_PIX_FMT_NV12;\n> >> +\toutputFormat.planesCount = 2;\n> >>\n> >> -\tret = cio2->setFormat(&devFormat);\n> >> +\tret = output->setFormat(&outputFormat);\n> >>  \tif (ret)\n> >>  \t\treturn ret;\n> >>\n> >> -\tLOG(IPU3, Info) << cio2->driverName() << \": \"\n> >> -\t\t\t<< devFormat.width << \"x\" << devFormat.height\n> >> -\t\t\t<< \"- 0x\" << std::hex << devFormat.fourcc << \" planes: \"\n> >> -\t\t\t<< devFormat.planes;\n> >> +\tLOG(IPU3, Debug)\n> >> +\t\t<< \"ImgU output format = \" << outputFormat.toString();\n> >> +\n> >> +\tret = viewfinder->setFormat(&outputFormat);\n> >> +\tif (ret)\n> >> +\t\treturn ret;\n> >>\n> >>  \treturn 0;\n> >>  }\n> >> @@ -666,6 +705,125 @@ void PipelineHandlerIPU3::deleteCIO2(CIO2Device *cio2)\n> >>  \tdelete cio2->sensor;\n> >>  }\n> >>\n> >> +int PipelineHandlerIPU3::setImguFormat(ImgUDevice *imguDevice,\n> >> +\t\t\t\t       const StreamConfiguration &config,\n> >> +\t\t\t\t       Rectangle *rect)\n> >> +{\n> >> +\tV4L2Subdevice *imgu = imguDevice->imgu;\n> >> +\tint ret;\n> >> +\n> >> +\t/*\n> >> +\t * Configure the 'imgu' subdevice with the requested sizes.\n> >\n> > s/'imgu'/ImgU/ ?\n> >\n> >> +\t *\n> >> +\t * FIXME: the IPU3 driver implementation shall be changed to use the\n> >> +\t * actual input sizes as 'imgu input' subdevice sizes, and use the\n> >> +\t * desired output sizes to configure the crop/compose rectangles.  The\n> >> +\t * current implementation uses output sizes as 'imgu input' sizes, and\n> >> +\t * uses the input dimension to configure the crop/compose rectangles,\n> >> +\t * which contradicts the V4L2 specifications.\n> >\n> > s/specifications/specification/\n> >\n> >> +\t */\n> >> +\tret = imgu->setCrop(ImgUDevice::PAD_INPUT, rect);\n> >> +\tif (ret)\n> >> +\t\treturn ret;\n> >> +\n> >> +\tret = imgu->setCompose(ImgUDevice::PAD_INPUT, rect);\n> >> +\tif (ret)\n> >> +\t\treturn ret;\n> >> +\n> >> +\tLOG(IPU3, Debug)\n> >> +\t\t<< \"ImgU input feeder and BDS rectangle = (0,0)/\"\n> >> +\t\t<< rect->w << \"x\" << rect->h;\n> >\n> > toString() for our Rectangle class ?\n> \n> I'll add a patch\n> \n> >> +\n> >> +\tV4L2SubdeviceFormat imguFormat = {};\n> >> +\timguFormat.width = config.width;\n> >> +\timguFormat.height = config.height;\n> >> +\timguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;\n> >> +\n> >> +\tret = imgu->setFormat(ImgUDevice::PAD_INPUT, &imguFormat);\n> >> +\tif (ret)\n> >> +\t\treturn ret;\n> >> +\n> >> +\tret = imgu->setFormat(ImgUDevice::PAD_OUTPUT, &imguFormat);\n> >> +\tif (ret)\n> >> +\t\treturn ret;\n> >> +\n> >> +\tLOG(IPU3, Debug)\n> >> +\t\t<< \"ImgU GDC format = \" << imguFormat.toString();\n> >> +\n> >> +\tret = imgu->setFormat(ImgUDevice::PAD_VF, &imguFormat);\n> >> +\tif (ret)\n> >> +\t\treturn ret;\n> >> +\n> >> +\tret = imgu->setFormat(ImgUDevice::PAD_STAT, &imguFormat);\n> >> +\tif (ret)\n> >> +\t\treturn ret;\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +int PipelineHandlerIPU3::setCIO2Format(CIO2Device *cio2Device,\n> >> +\t\t\t\t       const StreamConfiguration &config,\n> >> +\t\t\t\t       V4L2SubdeviceFormat *format)\n> >> +{\n> >\n> > We're starting to get quite some code associated with the CIO2Device. I\n> > know I requested for CIO2Device to be a struct without methods, but I\n> > wonder if that was a mistake :-(\n> >\n> > I would also rename the function, maybe to configure(). setFormat()\n> > hints that it sets the format passed as an argument, which is not the\n> > case here, the format parameter is an output.\n> >\n> >> +\tunsigned int imageSize = config.width * config.height;\n> >> +\tV4L2Subdevice *sensor = cio2Device->sensor;\n> >> +\tV4L2Subdevice *csi2 = cio2Device->csi2;\n> >> +\tunsigned int best = ~0;\n> >> +\tbool found = false;\n> >> +\tint ret;\n> >> +\n> >> +\tconst FormatEnum formats = sensor->formats(0);\n> >> +\tfor (auto it = formats.begin(); it != formats.end(); ++it) {\n> >> +\t\t/* Only consider formats consumable by the CIO2 unit. */\n> >> +\t\tint cio2Code = mediaBusToCIO2Format(it->first);\n> >> +\t\tif (cio2Code == -EINVAL)\n> >\n> > You could write\n> >\n> > \t\tif (mediaBusToCIO2Format(it->first) == -EINVAL)\n> >\n> > (and I would check for < 0 instead of == -EINVAL to avoid surprises if\n> > we later change that method)\n> >\n> >> +\t\t\tcontinue;\n> >> +\n> >> +\t\tfor (const SizeRange &size : it->second) {\n> >> +\t\t\t/*\n> >> +\t\t\t * Only select formats bigger than the requested sizes\n> >> +\t\t\t * as the IPU3 cannot up-scale.\n> >> +\t\t\t */\n> >> +\t\t\tif (size.maxWidth < config.width ||\n> >> +\t\t\t    size.maxHeight < config.height)\n> >> +\t\t\t\tcontinue;\n> >> +\n> >> +\t\t\tunsigned int diff = size.maxWidth * size.maxHeight\n> >> +\t\t\t\t\t  - imageSize;\n> >> +\t\t\tif (diff >= best)\n> >> +\t\t\t\tcontinue;\n> >> +\n> >> +\t\t\tbest = diff;\n> >> +\t\t\tfound = true;\n> >> +\n> >> +\t\t\tformat->width = size.maxWidth;\n> >> +\t\t\tformat->height = size.maxHeight;\n> >> +\t\t\tformat->mbus_code = it->first;\n> >> +\t\t}\n> >> +\t}\n> >\n> > Open question, should we do this, or always pick the largest size from\n> > the sensor and scale in the ImgU ?\n> >\n> \n> Good question... I cannot tell the pros and cons actually... why would\n> you think it is better?\n\nThe ImgU scaler should be of better quality than the sensor scaler, but\nscaling down on the sensor lowers the required bandwidth, and\npotentially increases the maximum frame rate.\n\n> >> +\tif (!found) {\n> >> +\t\tLOG(IPU3, Error)\n> >> +\t\t\t<< \"Unable to find image format suitable to produce: \"\n> >> +\t\t\t<< config.width << \"x\" << config.height\n> >> +\t\t\t<< \"- 0x\" << std::hex << std::setfill('0')\n> >\n> > Spaces before and after dash, or none at all.\n> >\n> >> +\t\t\t<< std::setw(8) << config.pixelFormat;\n> >\n> > Again a shame we can't use toString(). Do you use that helper at all ?\n> > :-)\n> >\n> >> +\t\treturn -EINVAL;\n> >> +\t}\n> >> +\n> >> +\t/* Apply the selected format to the sensor and the CSI-2 receiver. */\n> >> +\tret = sensor->setFormat(0, format);\n> >> +\tif (ret)\n> >> +\t\treturn ret;\n> >> +\n> >> +\tret = csi2->setFormat(0, format);\n> >> +\tif (ret)\n> >> +\t\treturn ret;\n> >> +\n> >> +\tLOG(IPU3, Debug) << \"CIO2 Image format: \" << format->toString();\n> >\n> > Ah yes you use it here.\n> >\n> >> +\n> >> +\treturn 0;\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":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C9001610B6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 23 Mar 2019 14:00:47 +0100 (CET)","from pendragon.ideasonboard.com\n\t(p5269001-ipngn11702marunouchi.tokyo.ocn.ne.jp [114.158.195.1])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3D07349;\n\tSat, 23 Mar 2019 14:00:45 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1553346047;\n\tbh=aJRvdT+nm8PiOdnOX7HUJNLFacy7Okhu4qqicU5xq+k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=STOYN8otFL7j2rqRhUfELdTUEh6EkKzMrcJOE3E0VE5rwBdeLqIhKRvoFQR4U/6Jj\n\tNOR9r7hl/as+tkdc+NRz7PO4Ihnk+oqpJIymJ+ycGIx8ve41irgl5WH/sJNyKZsbCy\n\tkz1rwr6fHpw+SIrTYuui1aTEHT2VancAyRZ3QktM=","Date":"Sat, 23 Mar 2019 15:00:34 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190323130034.GC4587@pendragon.ideasonboard.com>","References":"<20190320163055.22056-1-jacopo@jmondi.org>\n\t<20190320163055.22056-8-jacopo@jmondi.org>\n\t<20190321095418.GJ4615@pendragon.ideasonboard.com>\n\t<20190321193112.oxxel6mdvkcot3yq@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190321193112.oxxel6mdvkcot3yq@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 07/31] libcamera: ipu3: Propagate\n\timage format","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Sat, 23 Mar 2019 13:00:48 -0000"}}]