[libcamera-devel] [PATCH 04/10] libcamera: ipu3: Propagate image format
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Mar 10 14:14:03 CET 2019
Hi Jacopo,
On Sat, Mar 09, 2019 at 09:58:16PM +0100, Jacopo Mondi wrote:
> On Sun, Mar 03, 2019 at 12:44:23AM +0200, Laurent Pinchart wrote:
> > 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)
>
> More code in the framework, less in pipeline handlers: it's a good idea!
>
> >> +
> >> + 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 ?
>
> Ack
>
> >> + *
> >> + * 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.
>
> As we have clarified offline I'll make this Debug and will make this
> and the others more user-consumable.
>
> >> +
> >> /*
> >> * 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 ?
>
> Re-reading the comment, I got why you got mis-lead... I'll change
> this. Suggestions welcome for the method 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.
>
> Ah! Isn't the output of the CIO2 device IPU3 specific and fixed?
> That's not what the sensor provides, but the CIO2 produced one
> (for which there are a few versions, which are different permutations
> of the RGB bayer components, but I don't think that' too relevant)
The CIO2 writes a custom packing of 10-bit Bayer data to memory (25
pixels stored in 32 bytes if I remember correctly). As far as I know it
doesn't reorder Bayer components, so you'll have to pick the
V4L2_PIX_FMT_IPU3_S[RGB]*10 variant that corresponds to the order
produced by the sensor.
> >> + 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 ?
>
> See my comments on the previous patch.
>
> >> + 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.
>
> Ack. Personal tastes or some reason I'm missing?
>
> >> + 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.
>
> Agreed. Let's always assume sensor drivers are well written (creepy
> laughs in background..)
:-) Well, sanity checks are nice, but I'd move them to match() time to
fail early instead of creating a camera that won't be able to work. Some
checks could even be moved to the V4L2 device and subdevice classes.
> >> + 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.
>
> Ack
>
> >> + 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 ?
>
> I'll try to move most of this to geometry
>
> >> + if (diff >= best)
> >> + continue;
> >> +
> >> + best = diff;
> >> + found = true;
> >
> > No need for the found variable, you can just check best != -1.
>
> Correct, this probably answers my above question.
>
> >> +
> >> + 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