[libcamera-devel] [PATCH 04/10] libcamera: ipu3: Propagate image format
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Mar 2 23:44:23 CET 2019
Hi Jacopo,
Thank you for the patch.
On Thu, Feb 28, 2019 at 09:04:04PM +0100, Jacopo Mondi wrote:
> Apply the requested image format to the sensor device, and apply the
> adjusted one to the CIO2 device, the ImgU subdevice and its input and
> output video devices.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> src/libcamera/pipeline/ipu3/ipu3.cpp | 257 +++++++++++++++++++++++----
> 1 file changed, 224 insertions(+), 33 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 9fa59c1bc97e..1e89e57f628b 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -8,11 +8,14 @@
> #include <memory>
> #include <vector>
>
> +#include <linux/media-bus-format.h>
> +
> #include <libcamera/camera.h>
> #include <libcamera/request.h>
> #include <libcamera/stream.h>
>
> #include "device_enumerator.h"
> +#include "geometry.h"
> #include "log.h"
> #include "media_device.h"
> #include "pipeline_handler.h"
> @@ -106,6 +109,29 @@ private:
> PipelineHandler::cameraData(camera));
> }
>
> + void printDevFormat(const V4L2Device *dev, const V4L2DeviceFormat &fmt)
> + {
> + LOG(IPU3, Info)
I think this is way too verbose in general.
> + << dev->deviceNode() << ": " << fmt.width << "x"
> + << fmt.height << "- 0x" << std::hex << fmt.fourcc
> + << " planes: " << fmt.planesCount;
> + }
> +
> + void printSubdevFormat(const V4L2Subdevice *dev, unsigned int pad,
> + const V4L2SubdeviceFormat &fmt)
> + {
> + LOG(IPU3, Info)
> + << "'" << dev->deviceName() << "':" << pad << " = "
> + << fmt.width << "x" << fmt.height << " - 0x"
> + << std::hex << fmt.mbus_code;
> + }
How about adding a std::string toString() const method to the
V4L2DeviceFormat and V4L2SubdeviceFormat classes ? print methods in the
PipelineHandlerIPU3 class aren't very nice :-/ (especially with the
hardcoded log level)
> +
> + int setImguFormat(V4L2Subdevice *imgu,
> + const StreamConfiguration &config,
> + Rectangle *rect);
> + int setSensorFormat(V4L2Subdevice *sensor,
> + const StreamConfiguration &config,
> + V4L2SubdeviceFormat *format);
> int linkImgu(ImguDevice *imgu);
>
> V4L2Device *openDevice(MediaDevice *media, std::string &name);
> @@ -186,14 +212,29 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
> std::map<Stream *, StreamConfiguration> &config)
> {
> IPU3CameraData *data = cameraData(camera);
> - StreamConfiguration *cfg = &config[&data->stream_];
> - V4L2Subdevice *sensor = data->cio2.sensor;
> + const StreamConfiguration &cfg = config[&data->stream_];
> V4L2Subdevice *csi2 = data->cio2.csi2;
> V4L2Device *cio2 = data->cio2.output;
> - V4L2SubdeviceFormat subdevFormat = {};
> - V4L2DeviceFormat devFormat = {};
> int ret;
>
> + /*
> + * Verify that the requested size respects the IPU3 alignement
> + * requirements: image size shall be 8-aligned in width and 4-aligned
> + * in height.
Maybe "the image width shall be a multiple of 8 pixels and its height a
multiple of 4 pixels" to clarify we're talking about pixels, not bytes ?
> + *
> + * TODO: consider the BDS scaling factor requirements: "the downscaling
> + * factor must be an integer value multiple of 1/32"
> + */
> + if (cfg.width % 8 || cfg.height % 4) {
> + LOG(IPU3, Error) << "Stream format not support: bad alignement";
> + return -EINVAL;
> + }
> +
> + LOG(IPU3, Info)
> + << "Camera :'" << camera->name() << " - Stream format: "
> + << cfg.width << "x" << cfg.height << " - "
> + << std::hex << cfg.pixelFormat;
Logging is important, but this is all way too verbose. Please consider
for every log message if it's needed, and if it is, what the appropriate
log level should be. The messages should also be more informative. This
message, for instance, should explicitly state that it prints the
requested format, and also mention which stream it applies to. You
should also move it before the alignment check.
> +
> /*
> * TODO: dynamically assign ImgU devices; as of now, with a single
> * stream supported, always use 'imgu0'.
> @@ -203,52 +244,73 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
> if (ret)
> return ret;
>
> + V4L2Device *viewfinder = data->imgu->viewfinder;
> + V4L2Subdevice *sensor = data->cio2.sensor;
> + V4L2Device *output = data->imgu->output;
> + V4L2Subdevice *imgu = data->imgu->imgu;
> + V4L2Device *input = data->imgu->input;
> +
> /*
> - * FIXME: as of now, the format gets applied to the sensor and is
> - * propagated along the pipeline. It should instead be applied on the
> - * capture device and the sensor format calculated accordingly.
> + * Pass the requested output image size to the sensor and get back the
> + * adjusted one to be propagated to the CIO2 device and to the ImgU
> + * input.
> */
If the sensor driver returns the closest match, you may get a size
smaller than requested, which would require up-scaling. I'm not sure if
the IPU3 supports that, but in any case it may not be a very good idea.
I think you should instead iterate over the sizes supported by the
sensor and pick the best one yourself. We may also want to use cropping
at some point, but that can be left for later.
And now that I've written this, I realized that your setSensorFormat()
method has such an iteration. Could you update the above comment
accordingly, and possibly rename setSensorFormat() to a more descriptive
name ?
> + V4L2SubdeviceFormat sensorFormat = {};
> + ret = setSensorFormat(sensor, cfg, &sensorFormat);
> + if (ret) {
> + LOG(IPU3, Error) << "Stream format not supported: ";
Looks like you're missing part of the message. I would also move it to
the setSensorFormat() function in order to print the exact reason of the
error.
> + return ret;
> + }
> + printSubdevFormat(sensor, 0, sensorFormat);
>
> - ret = sensor->getFormat(0, &subdevFormat);
> + ret = csi2->setFormat(0, &sensorFormat);
> if (ret)
> return ret;
> -
> - subdevFormat.width = cfg->width;
> - subdevFormat.height = cfg->height;
> - ret = sensor->setFormat(0, &subdevFormat);
> + printSubdevFormat(csi2, 0, sensorFormat);
> +
> + /* Apply the CIO2 image format to the CIO2 output and ImgU input. */
> + V4L2DeviceFormat cio2Format = {};
> + cio2Format.width = sensorFormat.width;
> + cio2Format.height = sensorFormat.height;
> + cio2Format.fourcc = V4L2_PIX_FMT_IPU3_SGRBG10;
Let's not hardcode a particular bayer pattern, please pick the
appropriate one based on what the sensor provides.
> + cio2Format.planesCount = 1;
> + ret = cio2->setFormat(&cio2Format);
> if (ret)
> return ret;
> + printDevFormat(cio2, cio2Format);
>
> - /* Return error if the requested format cannot be applied to sensor. */
> - if (subdevFormat.width != cfg->width ||
> - subdevFormat.height != cfg->height) {
> - LOG(IPU3, Error)
> - << "Failed to apply image format "
> - << subdevFormat.width << "x" << subdevFormat.height
> - << " - got: " << cfg->width << "x" << cfg->height;
> - return -EINVAL;
> - }
> -
> - ret = csi2->setFormat(0, &subdevFormat);
> + ret = input->setFormat(&cio2Format);
> if (ret)
> return ret;
> -
> - ret = cio2->getFormat(&devFormat);
> + printDevFormat(input, cio2Format);
> +
> + /* Apply pad formats and crop/compose rectangle to the ImgU. */
> + Rectangle rect = {
> + .x = 0,
> + .y = 0,
> + .w = cio2Format.width,
> + .h = cio2Format.height,
> + };
> + ret = setImguFormat(imgu, cfg, &rect);
> if (ret)
> return ret;
>
> - devFormat.width = subdevFormat.width;
> - devFormat.height = subdevFormat.height;
> - devFormat.fourcc = cfg->pixelFormat;
> + /* Apply the format to the ImgU output and viewfinder devices. */
As commented on the previous patch, with a single stream supported, do
you really need both outputs ?
> + V4L2DeviceFormat outputFormat = {};
> + outputFormat.width = cfg.width;
> + outputFormat.height = cfg.height;
> + outputFormat.fourcc = V4L2_PIX_FMT_NV12;
> + outputFormat.planesCount = 2;
>
> - ret = cio2->setFormat(&devFormat);
> + ret = output->setFormat(&outputFormat);
> if (ret)
> return ret;
> + printDevFormat(output, outputFormat);
>
> - LOG(IPU3, Info) << cio2->driverName() << ": "
> - << devFormat.width << "x" << devFormat.height
> - << "- 0x" << std::hex << devFormat.fourcc << " planes: "
> - << devFormat.planes;
> + ret = viewfinder->setFormat(&outputFormat);
> + if (ret)
> + return ret;
> + printDevFormat(viewfinder, outputFormat);
>
> return 0;
> }
> @@ -409,6 +471,135 @@ error_release_mdev:
> return false;
> }
>
> +int PipelineHandlerIPU3::setImguFormat(V4L2Subdevice *imgu,
> + const StreamConfiguration &config,
> + Rectangle *rect)
> +{
> + int ret;
> +
> + /*
> + * Configure the 'imgu' subdevice with the requested sizes.
> + *
> + * FIXME: the IPU3 driver implementation shall be changed to use the
> + * actual input sizes as 'imgu input' subdevice sizes, and use the
> + * desired output sizes to configure the crop/compose rectangles. The
> + * current implementation uses output sizes as 'imgu input' sizes, and
> + * uses the input dimension to configure the crop/compose rectangles,
> + * which contradicts the V4L2 specifications.
> + */
> + ret = imgu->setCrop(IMGU_PAD_INPUT, rect);
> + if (ret)
> + return ret;
> +
> + LOG(IPU3, Info)
> + << "'" << imgu->deviceName() << "':" << IMGU_PAD_INPUT
> + << " = crop: (0,0)/" << rect->w << "x" << rect->h;
> +
> + ret = imgu->setCompose(IMGU_PAD_INPUT, rect);
> + if (ret)
> + return ret;
> +
> + LOG(IPU3, Info)
> + << "'" << imgu->deviceName() << "':" << IMGU_PAD_INPUT
> + << " = compose: (0,0)/" << rect->w << "x" << rect->h;
> +
> +
> + V4L2SubdeviceFormat imguFormat = {};
> + imguFormat.width = config.width;
> + imguFormat.height = config.height;
> + imguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;
> +
> + ret = imgu->setFormat(IMGU_PAD_INPUT, &imguFormat);
> + if (ret)
> + return ret;
> + printSubdevFormat(imgu, IMGU_PAD_INPUT, imguFormat);
> +
> + ret = imgu->setFormat(IMGU_PAD_OUTPUT, &imguFormat);
> + if (ret)
> + return ret;
> + printSubdevFormat(imgu, IMGU_PAD_OUTPUT, imguFormat);
> +
> + ret = imgu->setFormat(IMGU_PAD_VF, &imguFormat);
> + if (ret)
> + return ret;
> + printSubdevFormat(imgu, IMGU_PAD_VF, imguFormat);
> +
> + ret = imgu->setFormat(IMGU_PAD_STAT, &imguFormat);
> + if (ret)
> + return ret;
> + printSubdevFormat(imgu, IMGU_PAD_STAT, imguFormat);
> +
> + return 0;
> +}
> +
> +int PipelineHandlerIPU3::setSensorFormat(V4L2Subdevice *sensor,
> + const StreamConfiguration &config,
> + V4L2SubdeviceFormat *format)
> +{
> + std::map<unsigned int, std::vector<SizeRange>> formats;
> + unsigned int best = ~0;
Please use -1 instead of ~0.
> + bool found = false;
> + int ret;
> +
> + formats = sensor->formats(0);
> + if (formats.empty()) {
> + /*
> + * If the format list is empty, try with the currently
> + * applied one.
> + */
This shouldn't happen.
> + ret = sensor->getFormat(0, format);
> + if (ret)
> + return ret;
> +
> + if (format->width < config.width ||
> + format->height < config.height)
> + return -EINVAL;
> +
> + return 0;
> + }
> +
> + /* Search for the best approximation the sensor can provide. */
> + auto it = formats.begin();
> + while (it != formats.end()) {
for loop.
> + for (SizeRange &size : it->second) {
> + if (size.maxWidth < config.width ||
> + size.maxHeight < config.height)
> + continue;
> +
> + unsigned int diff =
> + (abs(size.maxWidth - config.width) +
> + abs(size.maxHeight - config.height));
No need for abs() as you check that maxWidth >= config.width above.
Should you use the difference of area instead of the difference of
lengths ?
> + if (diff >= best)
> + continue;
> +
> + best = diff;
> + found = true;
No need for the found variable, you can just check best != -1.
> +
> + format->width = size.maxWidth;
> + format->height = size.maxHeight;
> + format->mbus_code = it->first;
> + }
> +
> + ++it;
> + }
> + if (!found)
> + return -EINVAL;
> +
> + ret = sensor->setFormat(0, format);
> + if (ret)
> + return ret;
> +
> + /*
> + * Make sure everything is all right and the format did not get
> + * adjusted.
> + */
> + if (format->width < config.width ||
> + format->height < config.height)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> /* Link entities in the ImgU unit to prepare for capture operations. */
> int PipelineHandlerIPU3::linkImgu(ImguDevice *imguDevice)
> {
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list